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

[Charlie Jenkins <charlie@rivosinc.com>]

On Wed, Dec 18, 2024 at 05:52:24PM -0800, Charlie Jenkins wrote:
> On Wed, Dec 18, 2024 at 05:20:15PM -0800, Ian Rogers wrote:
> > On Wed, Dec 18, 2024 at 2:32 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > >
> > > 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 need to write the patch set to delete libbfd from perf. IANAL but
> > the issue is that libbfd is part of binutils and GPLv3, while perf is
> > part of the Linux kernel and largely GPLv2. GPLv3 is incompatible with
> > GPLv2:
> > https://www.gnu.org/licenses/gpl-faq.html#AllCompatibility
> > While using dlopen means we're not linking against libbfd, we may
> > effectively be using it as a plugin which again GPLv3 (in my IANAL
> > opinion) wouldn't allow:
> > https://www.gnu.org/licenses/gpl-faq.en.html#GPLPlugins
> > Currently to get libbfd support in perf you need to be building the
> > binary yourself and add to the build BUILD_NONDISTRO=1. We do this as
> > part of our build testing but having all the #ifdef-ed libbfd code if
> > nothing else makes the code harder to understand.
> 
> Licensing is fun ;)
> 
> > 
> > > 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.
> > 
> > In the past I perceived there was hostility toward LLVM from the Linux
> > kernel community. I guess GPL was considered the special sauce as to
> > why Linux won and the BSDs hadn't, so the preference was to favor a
> > compiler that used the same license. I don't think that's true any
> > more and I think there's a lot of sense in using LLVM's libraries
> > rather than reinventing them in the perf tool, or using perhaps less
> > orthodox sources like libcapstone. I'm not a fan of the text output
> > processing stuff so getting rid of objdump and llvm-objdump support
> > would be good imo.
> 
> Yeah I agree. This test case did end up being interesting though as it
> unconvered this change in behavior of objdump on riscv, but that's
> tangential to the purpose of this test case. We need this patch on riscv
> to stop this test from failing, but it is also reasonable to approach
> this differently and not use objdump at all.

What's the next step here? Would you prefer to get rid of this test
entirely? I sent out a v3 that uses uname [1].

Link
https://lore.kernel.org/lkml/20241219-perf_fix_riscv_obj_reading-v3-1-a7d644dcfa50@rivosinc.com/
[1]

- Charlie

> 
> > 
> > Another area where I think we could lose a lot of code baggage is with
> > libunwind, as BPF support requires libelf which brings with it
> > libdwarves which when present means we don't use libunwind. I've heard
> > reports that libdwarves is slower, but I'm sure we can add caches to
> > speed it up which would likely benefit a range of people. I was kind
> > of hoping with all that deleted we may be able to get rid of the
> > majority of the arch directory, but the syscalltbl work is adding to
> > that directory :-)
> 
> Yeah... The syscalltbl work will add some extra parts to the arch
> directory. A lot of the additions are generic and a chunk of
> arch-specific ifdefs were able to be removed, but we still have the
> "problem" that not every architecture uses the shared syscall table and
> supports different syscalls.
> 
> - Charlie
> 
> > 
> > Thanks,
> > Ian