[PATCH v2] tools: perf: tests: Fix code reading for riscv

[Ian Rogers]

On Wed, Dec 18, 2024 at 1:02 PM Charlie Jenkins <charlie at rivosinc.com> wrote:
>
> On Wed, Dec 18, 2024 at 11:23:51AM -0800, Ian Rogers wrote:
> > On Wed, Dec 18, 2024 at 10:41 AM Arnaldo Carvalho de Melo
> > <acme at kernel.org> wrote:
> > >
> > > On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> > > > On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > > > > On Tue, Dec 17, 2024 at 3:52 PM Charlie Jenkins <charlie at rivosinc.com> wrote:
> > > > > > After binutils commit e43d876 which was first included in binutils 2.41,
> > > > > > riscv no longer supports dumping in the middle of instructions. Increase
> > > > > > the objdump window by 2-bytes to ensure that any instruction that sits
> > > > > > on the boundary of the specified stop-address is not cut in half.
> > >
> > > > > > Signed-off-by: Charlie Jenkins <charlie at rivosinc.com>
> > >
> > > > > Reviewed-by: Ian Rogers <irogers at google.com>
> > >
> > > > > > A binutils patch has been sent as well to fix this in objdump [1].
> > >
> > > > > > Link: https://sourceware.org/pipermail/binutils/2024-December/138139.html [1]
> > >
> > > > > > Changes in v2:
> > > > > > - Do objdump version detection at runtime (Ian)
> > > > > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> > >
> > > > > > --- a/tools/perf/tests/code-reading.c
> > > > > > @@ -183,9 +244,30 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf,
> > > > > >         const char *fmt;
> > > > > >         FILE *f;
> > > > > >         int ret;
> > > > > > +       u64 stop_address = addr + len;
> > > > > > +
> > > > > > +       if (IS_ENABLED(__riscv)) {
> > >
> > > > > Not sure if there is a consistency issue here. Elsewhere we're just
> > > > > using ifdef, such as:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
> > >
> > > > I don't have any strong feelings about that. I can change it to be an
> > > > ifdef. On other lists I have been told to use IS_ENABLED whenever
> > > > possible, but it's only a small difference.
> > >
> > > Can't we just use uname here?
> > >
> > > So that we don't use kconfig.h since its not used in tools/perf/ and
> > > makes it looks like perf is in lockstep with the kernel source tree
> > > version it was compiled from?
> > >
> > > $ git grep kconfig.h tools/perf/
> > > $
> > >
> > > BTW, what would happen if I collected a perf.data file on x86_64 and
> > > would read it in a RiscV machine with such a objdump version? The same
> > > problem?
> >
> > This code is in tests hence thinking that a separate fix is needed for
> > that problem. Hopefully the use of elf machine/flags tackles it:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n25
> > We are getting somewhat disassembler heavy. We have llvm as a library,
> > capstone as a library, binutils objdump and llvm objdump. Given the
> > pain with parsing text, could we lose the objdumps? Similarly for
> > addr2line?
>
> Are you suggesting to remove this test case entirely to get rid of the
> objdump dependency? The goal of this test case seems to be to check
> objdump and perf return the same data, so it doesn't seem like there
> would be an alternative to using objdump.

I can imagine having an objdump dependency for a test but not for some
more core like `perf annotate`. We have to do weird things when
parsing text, like this code I'm not proud of:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/srcline.c?h=perf-tools-next#n523
The issue with that code is that LLVM objdump has changed its output
in newer versions to be closer to binutils objdump. Did that break
perf? Maybe it just broke what our variables think is an LLVM objdump,
but things aren't really broken. This kind of issue doesn't occur with
a library, although the differing needs of library versions is a real
thing.

Thanks,
Ian