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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z2NNcy1jCLqb62h2@ghost>
Date: Wed, 18 Dec 2024 14:32:19 -0800
From: Charlie Jenkins <charlie@...osinc.com>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Namhyung Kim <namhyung@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

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@...osinc.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@...nel.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@...osinc.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@...osinc.com>
> > > >
> > > > > > Reviewed-by: Ian Rogers <irogers@...gle.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