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

[Charlie Jenkins <charlie@rivosinc.com>]

On Wed, Dec 18, 2024 at 02:13:20PM -0800, Ian Rogers wrote:
> On Wed, Dec 18, 2024 at 1:02 PM Charlie Jenkins <charlie@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@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@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@rivosinc.com>
> > > >
> > > > > > Reviewed-by: Ian Rogers <irogers@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.

Yeah doing the parsing of the text output is not ideal... For this test
case it should be possible to dynamically link against libbfd. I would
guess something similar could be done with llvm-objdump but I am less
familiar with that. I don't know if that's a good path to go down
though.

- Charlie

> 
> Thanks,
> Ian