| Message ID | 20250201071455.718247-1-irogers@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | perf: Support multiple system call tables in the build | expand |
On Fri, Jan 31, 2025 at 11:15 PM Ian Rogers <irogers@google.com> wrote: > > This work builds on the clean up of system call tables and removal of > libaudit by Charlie Jenkins <charlie@rivosinc.com>. > > The system call table in perf trace is used to map system call numbers > to names and vice versa. Prior to these changes, a single table > matching the perf binary's build was present. The table would be > incorrect if tracing say a 32-bit binary from a 64-bit version of > perf, the names and numbers wouldn't match. > > Change the build so that a single system call file is built and the > potentially multiple tables are identifiable from the ELF machine type > of the process being examined. To determine the ELF machine type, the > executable's header is read from /proc/pid/exe with fallbacks to using > the perf's binary type when unknown. > > Remove some runtime types used by the system call tables and make > equivalents generated at build time. > > Ian Rogers (7): > perf syscalltble: Remove syscall_table.h > perf trace: Reorganize syscalls > perf syscalltbl: Remove struct syscalltbl > perf thread: Add support for reading the e_machine type for a thread > perf trace beauty: Add syscalltbl.sh generating all system call tables > perf syscalltbl: Use lookup table containing multiple architectures > perf build: Remove Makefile.syscalls If you are looking for the improvement this series achieves, patch 6 has sample before and after output: https://lore.kernel.org/lkml/20250201071455.718247-7-irogers@google.com/ Thanks, Ian
On Sat, Feb 1, 2025 at 12:51 AM Ian Rogers <irogers@google.com> wrote: > > On Fri, Jan 31, 2025 at 11:15 PM Ian Rogers <irogers@google.com> wrote: > > > > This work builds on the clean up of system call tables and removal of > > libaudit by Charlie Jenkins <charlie@rivosinc.com>. > > > > The system call table in perf trace is used to map system call numbers > > to names and vice versa. Prior to these changes, a single table > > matching the perf binary's build was present. The table would be > > incorrect if tracing say a 32-bit binary from a 64-bit version of > > perf, the names and numbers wouldn't match. > > > > Change the build so that a single system call file is built and the > > potentially multiple tables are identifiable from the ELF machine type > > of the process being examined. To determine the ELF machine type, the > > executable's header is read from /proc/pid/exe with fallbacks to using > > the perf's binary type when unknown. > > > > Remove some runtime types used by the system call tables and make > > equivalents generated at build time. > > > > Ian Rogers (7): > > perf syscalltble: Remove syscall_table.h > > perf trace: Reorganize syscalls > > perf syscalltbl: Remove struct syscalltbl > > perf thread: Add support for reading the e_machine type for a thread > > perf trace beauty: Add syscalltbl.sh generating all system call tables > > perf syscalltbl: Use lookup table containing multiple architectures > > perf build: Remove Makefile.syscalls > > If you are looking for the improvement this series achieves, patch 6 > has sample before and after output: > https://lore.kernel.org/lkml/20250201071455.718247-7-irogers@google.com/ I think there is a follow up to these patches to clean up the notion of "arch" and its "name" member: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/disasm.h?h=perf-tools-next#n20 The name on x86-64 has the value "x86" and isn't known to be i386 or x86-64, causing ambiguity to work around: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/disasm.c?h=perf-tools-next#n1898 It feels likely that rather than recording (in env) a string based on uname and bashing it around: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/env.c?h=perf-tools-next#n440 It would be better to record the e_machine values for the set of threads in the record, or just a set of potential e_machine values. We can get rid of name in arch, use e_machine from some dso helper, and then deal with this as appropriate without needing to carry around is_64bit to deal with ambiguity. A change like this would need its own feature flag, and helpers between the old name and e_machine so all notions of the old style name could be removed, etc. It may be convenient to duplicate arch tables for i386/x86-64 rather than have disasm's arch deal with a set of e_machines. Thanks, Ian
On Fri, Jan 31, 2025 at 11:14:48PM -0800, Ian Rogers wrote: > This work builds on the clean up of system call tables and removal of > libaudit by Charlie Jenkins <charlie@rivosinc.com>. > > The system call table in perf trace is used to map system call numbers > to names and vice versa. Prior to these changes, a single table > matching the perf binary's build was present. The table would be > incorrect if tracing say a 32-bit binary from a 64-bit version of > perf, the names and numbers wouldn't match. > > Change the build so that a single system call file is built and the > potentially multiple tables are identifiable from the ELF machine type > of the process being examined. To determine the ELF machine type, the > executable's header is read from /proc/pid/exe with fallbacks to using > the perf's binary type when unknown. > > Remove some runtime types used by the system call tables and make > equivalents generated at build time. Our approaches are very different but I sent out a patch to do this a couple of weeks ago [1]. Did you look at that and decide you didn't like the approach? - Charlie > > Ian Rogers (7): > perf syscalltble: Remove syscall_table.h > perf trace: Reorganize syscalls > perf syscalltbl: Remove struct syscalltbl > perf thread: Add support for reading the e_machine type for a thread > perf trace beauty: Add syscalltbl.sh generating all system call tables > perf syscalltbl: Use lookup table containing multiple architectures > perf build: Remove Makefile.syscalls > > tools/perf/Makefile.perf | 10 +- > tools/perf/arch/alpha/entry/syscalls/Kbuild | 2 - > .../alpha/entry/syscalls/Makefile.syscalls | 5 - > tools/perf/arch/alpha/include/syscall_table.h | 2 - > tools/perf/arch/arc/entry/syscalls/Kbuild | 2 - > .../arch/arc/entry/syscalls/Makefile.syscalls | 3 - > tools/perf/arch/arc/include/syscall_table.h | 2 - > tools/perf/arch/arm/entry/syscalls/Kbuild | 4 - > .../arch/arm/entry/syscalls/Makefile.syscalls | 2 - > tools/perf/arch/arm/include/syscall_table.h | 2 - > tools/perf/arch/arm64/entry/syscalls/Kbuild | 3 - > .../arm64/entry/syscalls/Makefile.syscalls | 6 - > tools/perf/arch/arm64/include/syscall_table.h | 8 - > tools/perf/arch/csky/entry/syscalls/Kbuild | 2 - > .../csky/entry/syscalls/Makefile.syscalls | 3 - > tools/perf/arch/csky/include/syscall_table.h | 2 - > .../perf/arch/loongarch/entry/syscalls/Kbuild | 2 - > .../entry/syscalls/Makefile.syscalls | 3 - > .../arch/loongarch/include/syscall_table.h | 2 - > tools/perf/arch/mips/entry/syscalls/Kbuild | 2 - > .../mips/entry/syscalls/Makefile.syscalls | 5 - > tools/perf/arch/mips/include/syscall_table.h | 2 - > tools/perf/arch/parisc/entry/syscalls/Kbuild | 3 - > .../parisc/entry/syscalls/Makefile.syscalls | 6 - > .../perf/arch/parisc/include/syscall_table.h | 8 - > tools/perf/arch/powerpc/entry/syscalls/Kbuild | 3 - > .../powerpc/entry/syscalls/Makefile.syscalls | 6 - > .../perf/arch/powerpc/include/syscall_table.h | 8 - > tools/perf/arch/riscv/entry/syscalls/Kbuild | 2 - > .../riscv/entry/syscalls/Makefile.syscalls | 4 - > tools/perf/arch/riscv/include/syscall_table.h | 8 - > tools/perf/arch/s390/entry/syscalls/Kbuild | 2 - > .../s390/entry/syscalls/Makefile.syscalls | 5 - > tools/perf/arch/s390/include/syscall_table.h | 2 - > tools/perf/arch/sh/entry/syscalls/Kbuild | 2 - > .../arch/sh/entry/syscalls/Makefile.syscalls | 4 - > tools/perf/arch/sh/include/syscall_table.h | 2 - > tools/perf/arch/sparc/entry/syscalls/Kbuild | 3 - > .../sparc/entry/syscalls/Makefile.syscalls | 5 - > tools/perf/arch/sparc/include/syscall_table.h | 8 - > tools/perf/arch/x86/entry/syscalls/Kbuild | 3 - > .../arch/x86/entry/syscalls/Makefile.syscalls | 6 - > tools/perf/arch/x86/include/syscall_table.h | 8 - > tools/perf/arch/xtensa/entry/syscalls/Kbuild | 2 - > .../xtensa/entry/syscalls/Makefile.syscalls | 4 - > .../perf/arch/xtensa/include/syscall_table.h | 2 - > tools/perf/builtin-trace.c | 275 +++++++++++------- > tools/perf/scripts/Makefile.syscalls | 61 ---- > tools/perf/scripts/syscalltbl.sh | 86 ------ > tools/perf/trace/beauty/syscalltbl.sh | 274 +++++++++++++++++ > tools/perf/util/syscalltbl.c | 142 ++++----- > tools/perf/util/syscalltbl.h | 22 +- > tools/perf/util/thread.c | 50 ++++ > tools/perf/util/thread.h | 14 +- > 54 files changed, 598 insertions(+), 506 deletions(-) > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/alpha/include/syscall_table.h > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/arc/include/syscall_table.h > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/arm/include/syscall_table.h > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/arm64/include/syscall_table.h > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/csky/include/syscall_table.h > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/loongarch/include/syscall_table.h > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/mips/include/syscall_table.h > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/parisc/include/syscall_table.h > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/powerpc/include/syscall_table.h > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/riscv/include/syscall_table.h > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/s390/include/syscall_table.h > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/sh/include/syscall_table.h > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/sparc/include/syscall_table.h > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/x86/include/syscall_table.h > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Kbuild > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Makefile.syscalls > delete mode 100644 tools/perf/arch/xtensa/include/syscall_table.h > delete mode 100644 tools/perf/scripts/Makefile.syscalls > delete mode 100755 tools/perf/scripts/syscalltbl.sh > create mode 100755 tools/perf/trace/beauty/syscalltbl.sh > > -- > 2.48.1.362.g079036d154-goog >
On Mon, Feb 3, 2025 at 11:02 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > On Fri, Jan 31, 2025 at 11:14:48PM -0800, Ian Rogers wrote: > > This work builds on the clean up of system call tables and removal of > > libaudit by Charlie Jenkins <charlie@rivosinc.com>. > > > > The system call table in perf trace is used to map system call numbers > > to names and vice versa. Prior to these changes, a single table > > matching the perf binary's build was present. The table would be > > incorrect if tracing say a 32-bit binary from a 64-bit version of > > perf, the names and numbers wouldn't match. > > > > Change the build so that a single system call file is built and the > > potentially multiple tables are identifiable from the ELF machine type > > of the process being examined. To determine the ELF machine type, the > > executable's header is read from /proc/pid/exe with fallbacks to using > > the perf's binary type when unknown. > > > > Remove some runtime types used by the system call tables and make > > equivalents generated at build time. > > Our approaches are very different but I sent out a patch to do this a > couple of weeks ago [1]. > > Did you look at that and decide you didn't like the approach? Missing link? The patches generating a syscall(_32|_64)?.h landed. These changes take your changes and make it so that we just run the script once building a header file for all architectures. On x86 we then have the tables be guarded by ifdefs on i386 and x86-64. Then rather than the table just matching the host architecture the ELF machine is used for the executable running. Thanks, Ian > - Charlie > > > > > Ian Rogers (7): > > perf syscalltble: Remove syscall_table.h > > perf trace: Reorganize syscalls > > perf syscalltbl: Remove struct syscalltbl > > perf thread: Add support for reading the e_machine type for a thread > > perf trace beauty: Add syscalltbl.sh generating all system call tables > > perf syscalltbl: Use lookup table containing multiple architectures > > perf build: Remove Makefile.syscalls > > > > tools/perf/Makefile.perf | 10 +- > > tools/perf/arch/alpha/entry/syscalls/Kbuild | 2 - > > .../alpha/entry/syscalls/Makefile.syscalls | 5 - > > tools/perf/arch/alpha/include/syscall_table.h | 2 - > > tools/perf/arch/arc/entry/syscalls/Kbuild | 2 - > > .../arch/arc/entry/syscalls/Makefile.syscalls | 3 - > > tools/perf/arch/arc/include/syscall_table.h | 2 - > > tools/perf/arch/arm/entry/syscalls/Kbuild | 4 - > > .../arch/arm/entry/syscalls/Makefile.syscalls | 2 - > > tools/perf/arch/arm/include/syscall_table.h | 2 - > > tools/perf/arch/arm64/entry/syscalls/Kbuild | 3 - > > .../arm64/entry/syscalls/Makefile.syscalls | 6 - > > tools/perf/arch/arm64/include/syscall_table.h | 8 - > > tools/perf/arch/csky/entry/syscalls/Kbuild | 2 - > > .../csky/entry/syscalls/Makefile.syscalls | 3 - > > tools/perf/arch/csky/include/syscall_table.h | 2 - > > .../perf/arch/loongarch/entry/syscalls/Kbuild | 2 - > > .../entry/syscalls/Makefile.syscalls | 3 - > > .../arch/loongarch/include/syscall_table.h | 2 - > > tools/perf/arch/mips/entry/syscalls/Kbuild | 2 - > > .../mips/entry/syscalls/Makefile.syscalls | 5 - > > tools/perf/arch/mips/include/syscall_table.h | 2 - > > tools/perf/arch/parisc/entry/syscalls/Kbuild | 3 - > > .../parisc/entry/syscalls/Makefile.syscalls | 6 - > > .../perf/arch/parisc/include/syscall_table.h | 8 - > > tools/perf/arch/powerpc/entry/syscalls/Kbuild | 3 - > > .../powerpc/entry/syscalls/Makefile.syscalls | 6 - > > .../perf/arch/powerpc/include/syscall_table.h | 8 - > > tools/perf/arch/riscv/entry/syscalls/Kbuild | 2 - > > .../riscv/entry/syscalls/Makefile.syscalls | 4 - > > tools/perf/arch/riscv/include/syscall_table.h | 8 - > > tools/perf/arch/s390/entry/syscalls/Kbuild | 2 - > > .../s390/entry/syscalls/Makefile.syscalls | 5 - > > tools/perf/arch/s390/include/syscall_table.h | 2 - > > tools/perf/arch/sh/entry/syscalls/Kbuild | 2 - > > .../arch/sh/entry/syscalls/Makefile.syscalls | 4 - > > tools/perf/arch/sh/include/syscall_table.h | 2 - > > tools/perf/arch/sparc/entry/syscalls/Kbuild | 3 - > > .../sparc/entry/syscalls/Makefile.syscalls | 5 - > > tools/perf/arch/sparc/include/syscall_table.h | 8 - > > tools/perf/arch/x86/entry/syscalls/Kbuild | 3 - > > .../arch/x86/entry/syscalls/Makefile.syscalls | 6 - > > tools/perf/arch/x86/include/syscall_table.h | 8 - > > tools/perf/arch/xtensa/entry/syscalls/Kbuild | 2 - > > .../xtensa/entry/syscalls/Makefile.syscalls | 4 - > > .../perf/arch/xtensa/include/syscall_table.h | 2 - > > tools/perf/builtin-trace.c | 275 +++++++++++------- > > tools/perf/scripts/Makefile.syscalls | 61 ---- > > tools/perf/scripts/syscalltbl.sh | 86 ------ > > tools/perf/trace/beauty/syscalltbl.sh | 274 +++++++++++++++++ > > tools/perf/util/syscalltbl.c | 142 ++++----- > > tools/perf/util/syscalltbl.h | 22 +- > > tools/perf/util/thread.c | 50 ++++ > > tools/perf/util/thread.h | 14 +- > > 54 files changed, 598 insertions(+), 506 deletions(-) > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/alpha/include/syscall_table.h > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/arc/include/syscall_table.h > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/arm/include/syscall_table.h > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/arm64/include/syscall_table.h > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/csky/include/syscall_table.h > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/loongarch/include/syscall_table.h > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/mips/include/syscall_table.h > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/parisc/include/syscall_table.h > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/powerpc/include/syscall_table.h > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/riscv/include/syscall_table.h > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/s390/include/syscall_table.h > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/sh/include/syscall_table.h > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/sparc/include/syscall_table.h > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/x86/include/syscall_table.h > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Kbuild > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Makefile.syscalls > > delete mode 100644 tools/perf/arch/xtensa/include/syscall_table.h > > delete mode 100644 tools/perf/scripts/Makefile.syscalls > > delete mode 100755 tools/perf/scripts/syscalltbl.sh > > create mode 100755 tools/perf/trace/beauty/syscalltbl.sh > > > > -- > > 2.48.1.362.g079036d154-goog > >
On Mon, Feb 03, 2025 at 11:10:49AM -0800, Ian Rogers wrote: > On Mon, Feb 3, 2025 at 11:02 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > On Fri, Jan 31, 2025 at 11:14:48PM -0800, Ian Rogers wrote: > > > This work builds on the clean up of system call tables and removal of > > > libaudit by Charlie Jenkins <charlie@rivosinc.com>. > > > > > > The system call table in perf trace is used to map system call numbers > > > to names and vice versa. Prior to these changes, a single table > > > matching the perf binary's build was present. The table would be > > > incorrect if tracing say a 32-bit binary from a 64-bit version of > > > perf, the names and numbers wouldn't match. > > > > > > Change the build so that a single system call file is built and the > > > potentially multiple tables are identifiable from the ELF machine type > > > of the process being examined. To determine the ELF machine type, the > > > executable's header is read from /proc/pid/exe with fallbacks to using > > > the perf's binary type when unknown. > > > > > > Remove some runtime types used by the system call tables and make > > > equivalents generated at build time. > > > > Our approaches are very different but I sent out a patch to do this a > > couple of weeks ago [1]. > > > > Did you look at that and decide you didn't like the approach? > > Missing link? Whoops here is the link: https://lore.kernel.org/all/20250114-perf_syscall_arch_runtime-v1-1-5b304e408e11@rivosinc.com/ > > The patches generating a syscall(_32|_64)?.h landed. These changes > take your changes and make it so that we just run the script once > building a header file for all architectures. On x86 we then have the > tables be guarded by ifdefs on i386 and x86-64. Then rather than the > table just matching the host architecture the ELF machine is used for > the executable running. > > Thanks, > Ian > > > - Charlie > > > > > > > > Ian Rogers (7): > > > perf syscalltble: Remove syscall_table.h > > > perf trace: Reorganize syscalls > > > perf syscalltbl: Remove struct syscalltbl > > > perf thread: Add support for reading the e_machine type for a thread > > > perf trace beauty: Add syscalltbl.sh generating all system call tables > > > perf syscalltbl: Use lookup table containing multiple architectures > > > perf build: Remove Makefile.syscalls > > > > > > tools/perf/Makefile.perf | 10 +- > > > tools/perf/arch/alpha/entry/syscalls/Kbuild | 2 - > > > .../alpha/entry/syscalls/Makefile.syscalls | 5 - > > > tools/perf/arch/alpha/include/syscall_table.h | 2 - > > > tools/perf/arch/arc/entry/syscalls/Kbuild | 2 - > > > .../arch/arc/entry/syscalls/Makefile.syscalls | 3 - > > > tools/perf/arch/arc/include/syscall_table.h | 2 - > > > tools/perf/arch/arm/entry/syscalls/Kbuild | 4 - > > > .../arch/arm/entry/syscalls/Makefile.syscalls | 2 - > > > tools/perf/arch/arm/include/syscall_table.h | 2 - > > > tools/perf/arch/arm64/entry/syscalls/Kbuild | 3 - > > > .../arm64/entry/syscalls/Makefile.syscalls | 6 - > > > tools/perf/arch/arm64/include/syscall_table.h | 8 - > > > tools/perf/arch/csky/entry/syscalls/Kbuild | 2 - > > > .../csky/entry/syscalls/Makefile.syscalls | 3 - > > > tools/perf/arch/csky/include/syscall_table.h | 2 - > > > .../perf/arch/loongarch/entry/syscalls/Kbuild | 2 - > > > .../entry/syscalls/Makefile.syscalls | 3 - > > > .../arch/loongarch/include/syscall_table.h | 2 - > > > tools/perf/arch/mips/entry/syscalls/Kbuild | 2 - > > > .../mips/entry/syscalls/Makefile.syscalls | 5 - > > > tools/perf/arch/mips/include/syscall_table.h | 2 - > > > tools/perf/arch/parisc/entry/syscalls/Kbuild | 3 - > > > .../parisc/entry/syscalls/Makefile.syscalls | 6 - > > > .../perf/arch/parisc/include/syscall_table.h | 8 - > > > tools/perf/arch/powerpc/entry/syscalls/Kbuild | 3 - > > > .../powerpc/entry/syscalls/Makefile.syscalls | 6 - > > > .../perf/arch/powerpc/include/syscall_table.h | 8 - > > > tools/perf/arch/riscv/entry/syscalls/Kbuild | 2 - > > > .../riscv/entry/syscalls/Makefile.syscalls | 4 - > > > tools/perf/arch/riscv/include/syscall_table.h | 8 - > > > tools/perf/arch/s390/entry/syscalls/Kbuild | 2 - > > > .../s390/entry/syscalls/Makefile.syscalls | 5 - > > > tools/perf/arch/s390/include/syscall_table.h | 2 - > > > tools/perf/arch/sh/entry/syscalls/Kbuild | 2 - > > > .../arch/sh/entry/syscalls/Makefile.syscalls | 4 - > > > tools/perf/arch/sh/include/syscall_table.h | 2 - > > > tools/perf/arch/sparc/entry/syscalls/Kbuild | 3 - > > > .../sparc/entry/syscalls/Makefile.syscalls | 5 - > > > tools/perf/arch/sparc/include/syscall_table.h | 8 - > > > tools/perf/arch/x86/entry/syscalls/Kbuild | 3 - > > > .../arch/x86/entry/syscalls/Makefile.syscalls | 6 - > > > tools/perf/arch/x86/include/syscall_table.h | 8 - > > > tools/perf/arch/xtensa/entry/syscalls/Kbuild | 2 - > > > .../xtensa/entry/syscalls/Makefile.syscalls | 4 - > > > .../perf/arch/xtensa/include/syscall_table.h | 2 - > > > tools/perf/builtin-trace.c | 275 +++++++++++------- > > > tools/perf/scripts/Makefile.syscalls | 61 ---- > > > tools/perf/scripts/syscalltbl.sh | 86 ------ > > > tools/perf/trace/beauty/syscalltbl.sh | 274 +++++++++++++++++ > > > tools/perf/util/syscalltbl.c | 142 ++++----- > > > tools/perf/util/syscalltbl.h | 22 +- > > > tools/perf/util/thread.c | 50 ++++ > > > tools/perf/util/thread.h | 14 +- > > > 54 files changed, 598 insertions(+), 506 deletions(-) > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/alpha/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/arc/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/arm/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/arm64/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/csky/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/loongarch/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/mips/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/parisc/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/powerpc/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/riscv/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/s390/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/sh/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/sparc/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/x86/include/syscall_table.h > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Kbuild > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Makefile.syscalls > > > delete mode 100644 tools/perf/arch/xtensa/include/syscall_table.h > > > delete mode 100644 tools/perf/scripts/Makefile.syscalls > > > delete mode 100755 tools/perf/scripts/syscalltbl.sh > > > create mode 100755 tools/perf/trace/beauty/syscalltbl.sh > > > > > > -- > > > 2.48.1.362.g079036d154-goog > > >
On Mon, Feb 3, 2025 at 11:15 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > On Mon, Feb 03, 2025 at 11:10:49AM -0800, Ian Rogers wrote: > > On Mon, Feb 3, 2025 at 11:02 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > > > On Fri, Jan 31, 2025 at 11:14:48PM -0800, Ian Rogers wrote: > > > > This work builds on the clean up of system call tables and removal of > > > > libaudit by Charlie Jenkins <charlie@rivosinc.com>. > > > > > > > > The system call table in perf trace is used to map system call numbers > > > > to names and vice versa. Prior to these changes, a single table > > > > matching the perf binary's build was present. The table would be > > > > incorrect if tracing say a 32-bit binary from a 64-bit version of > > > > perf, the names and numbers wouldn't match. > > > > > > > > Change the build so that a single system call file is built and the > > > > potentially multiple tables are identifiable from the ELF machine type > > > > of the process being examined. To determine the ELF machine type, the > > > > executable's header is read from /proc/pid/exe with fallbacks to using > > > > the perf's binary type when unknown. > > > > > > > > Remove some runtime types used by the system call tables and make > > > > equivalents generated at build time. > > > > > > Our approaches are very different but I sent out a patch to do this a > > > couple of weeks ago [1]. > > > > > > Did you look at that and decide you didn't like the approach? > > > > Missing link? > > Whoops here is the link: > > https://lore.kernel.org/all/20250114-perf_syscall_arch_runtime-v1-1-5b304e408e11@rivosinc.com/ I agree it is similar and progress, I think I prefer my changes :-) Anything in the arch directories is only going to be built given a Makefile SRCARCH: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/Build?h=perf-tools-next#n2 which means if we want the system call tables for say ARM on x86-64 we need to reinvent the build - this isn't a problem today but perhaps something to consider if we wanted a cross-architecture perf trace record. I'd like to do away entirely with the arch directory for this kind of reason. For example, if we run perf on a user space emulator (say a testing set up) and it sees AMD data fabric PMUs, logic for those PMUs currently lives in the arch directory for x86 and likely wasn't even built into the perf tool. We should be able to identify PMUs and act accordingly based on their names. In the case of the overloaded "cpu" PMU we can identify it with the CPUID. The less code in arch and the fewer architecture ifdefs, the better things should work cross-architecture, in testing scenarios, etc. We also win by getting code coverage without, for example, having to build and test on csky or super H. Your changes are using the string for the architecture name. I think that string is something we should get rid of: https://lore.kernel.org/lkml/CAP-5=fX2BtFzhGLCSqO1QszqfX=HT8RrTdG_5Ttp5Gw4seSstA@mail.gmail.com/ but there's no reason we couldn't clean that up on top of your change. The build with lots of separate Build/Makefiles is something of a pain to port/maintain in a bazel version of the build I maintain at Google (happy to open source if anyone cares). In these changes you just run a big script and get a big header file out the end, which largely matches the other perf trace beauty things we build. Thanks, Ian > > > > The patches generating a syscall(_32|_64)?.h landed. These changes > > take your changes and make it so that we just run the script once > > building a header file for all architectures. On x86 we then have the > > tables be guarded by ifdefs on i386 and x86-64. Then rather than the > > table just matching the host architecture the ELF machine is used for > > the executable running. > > > > Thanks, > > Ian > > > > > - Charlie > > > > > > > > > > > Ian Rogers (7): > > > > perf syscalltble: Remove syscall_table.h > > > > perf trace: Reorganize syscalls > > > > perf syscalltbl: Remove struct syscalltbl > > > > perf thread: Add support for reading the e_machine type for a thread > > > > perf trace beauty: Add syscalltbl.sh generating all system call tables > > > > perf syscalltbl: Use lookup table containing multiple architectures > > > > perf build: Remove Makefile.syscalls > > > > > > > > tools/perf/Makefile.perf | 10 +- > > > > tools/perf/arch/alpha/entry/syscalls/Kbuild | 2 - > > > > .../alpha/entry/syscalls/Makefile.syscalls | 5 - > > > > tools/perf/arch/alpha/include/syscall_table.h | 2 - > > > > tools/perf/arch/arc/entry/syscalls/Kbuild | 2 - > > > > .../arch/arc/entry/syscalls/Makefile.syscalls | 3 - > > > > tools/perf/arch/arc/include/syscall_table.h | 2 - > > > > tools/perf/arch/arm/entry/syscalls/Kbuild | 4 - > > > > .../arch/arm/entry/syscalls/Makefile.syscalls | 2 - > > > > tools/perf/arch/arm/include/syscall_table.h | 2 - > > > > tools/perf/arch/arm64/entry/syscalls/Kbuild | 3 - > > > > .../arm64/entry/syscalls/Makefile.syscalls | 6 - > > > > tools/perf/arch/arm64/include/syscall_table.h | 8 - > > > > tools/perf/arch/csky/entry/syscalls/Kbuild | 2 - > > > > .../csky/entry/syscalls/Makefile.syscalls | 3 - > > > > tools/perf/arch/csky/include/syscall_table.h | 2 - > > > > .../perf/arch/loongarch/entry/syscalls/Kbuild | 2 - > > > > .../entry/syscalls/Makefile.syscalls | 3 - > > > > .../arch/loongarch/include/syscall_table.h | 2 - > > > > tools/perf/arch/mips/entry/syscalls/Kbuild | 2 - > > > > .../mips/entry/syscalls/Makefile.syscalls | 5 - > > > > tools/perf/arch/mips/include/syscall_table.h | 2 - > > > > tools/perf/arch/parisc/entry/syscalls/Kbuild | 3 - > > > > .../parisc/entry/syscalls/Makefile.syscalls | 6 - > > > > .../perf/arch/parisc/include/syscall_table.h | 8 - > > > > tools/perf/arch/powerpc/entry/syscalls/Kbuild | 3 - > > > > .../powerpc/entry/syscalls/Makefile.syscalls | 6 - > > > > .../perf/arch/powerpc/include/syscall_table.h | 8 - > > > > tools/perf/arch/riscv/entry/syscalls/Kbuild | 2 - > > > > .../riscv/entry/syscalls/Makefile.syscalls | 4 - > > > > tools/perf/arch/riscv/include/syscall_table.h | 8 - > > > > tools/perf/arch/s390/entry/syscalls/Kbuild | 2 - > > > > .../s390/entry/syscalls/Makefile.syscalls | 5 - > > > > tools/perf/arch/s390/include/syscall_table.h | 2 - > > > > tools/perf/arch/sh/entry/syscalls/Kbuild | 2 - > > > > .../arch/sh/entry/syscalls/Makefile.syscalls | 4 - > > > > tools/perf/arch/sh/include/syscall_table.h | 2 - > > > > tools/perf/arch/sparc/entry/syscalls/Kbuild | 3 - > > > > .../sparc/entry/syscalls/Makefile.syscalls | 5 - > > > > tools/perf/arch/sparc/include/syscall_table.h | 8 - > > > > tools/perf/arch/x86/entry/syscalls/Kbuild | 3 - > > > > .../arch/x86/entry/syscalls/Makefile.syscalls | 6 - > > > > tools/perf/arch/x86/include/syscall_table.h | 8 - > > > > tools/perf/arch/xtensa/entry/syscalls/Kbuild | 2 - > > > > .../xtensa/entry/syscalls/Makefile.syscalls | 4 - > > > > .../perf/arch/xtensa/include/syscall_table.h | 2 - > > > > tools/perf/builtin-trace.c | 275 +++++++++++------- > > > > tools/perf/scripts/Makefile.syscalls | 61 ---- > > > > tools/perf/scripts/syscalltbl.sh | 86 ------ > > > > tools/perf/trace/beauty/syscalltbl.sh | 274 +++++++++++++++++ > > > > tools/perf/util/syscalltbl.c | 142 ++++----- > > > > tools/perf/util/syscalltbl.h | 22 +- > > > > tools/perf/util/thread.c | 50 ++++ > > > > tools/perf/util/thread.h | 14 +- > > > > 54 files changed, 598 insertions(+), 506 deletions(-) > > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/alpha/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/arc/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/arm/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/arm64/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/csky/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/loongarch/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/mips/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/parisc/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/powerpc/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/riscv/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/s390/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/sh/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/sparc/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/x86/include/syscall_table.h > > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Kbuild > > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Makefile.syscalls > > > > delete mode 100644 tools/perf/arch/xtensa/include/syscall_table.h > > > > delete mode 100644 tools/perf/scripts/Makefile.syscalls > > > > delete mode 100755 tools/perf/scripts/syscalltbl.sh > > > > create mode 100755 tools/perf/trace/beauty/syscalltbl.sh > > > > > > > > -- > > > > 2.48.1.362.g079036d154-goog > > > >
On Mon, Feb 03, 2025 at 11:39:01AM -0800, Ian Rogers wrote: > On Mon, Feb 3, 2025 at 11:15 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > On Mon, Feb 03, 2025 at 11:10:49AM -0800, Ian Rogers wrote: > > > On Mon, Feb 3, 2025 at 11:02 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > > > > > On Fri, Jan 31, 2025 at 11:14:48PM -0800, Ian Rogers wrote: > > > > > This work builds on the clean up of system call tables and removal of > > > > > libaudit by Charlie Jenkins <charlie@rivosinc.com>. > > > > > > > > > > The system call table in perf trace is used to map system call numbers > > > > > to names and vice versa. Prior to these changes, a single table > > > > > matching the perf binary's build was present. The table would be > > > > > incorrect if tracing say a 32-bit binary from a 64-bit version of > > > > > perf, the names and numbers wouldn't match. > > > > > > > > > > Change the build so that a single system call file is built and the > > > > > potentially multiple tables are identifiable from the ELF machine type > > > > > of the process being examined. To determine the ELF machine type, the > > > > > executable's header is read from /proc/pid/exe with fallbacks to using > > > > > the perf's binary type when unknown. > > > > > > > > > > Remove some runtime types used by the system call tables and make > > > > > equivalents generated at build time. > > > > > > > > Our approaches are very different but I sent out a patch to do this a > > > > couple of weeks ago [1]. > > > > > > > > Did you look at that and decide you didn't like the approach? > > > > > > Missing link? > > > > Whoops here is the link: > > > > https://lore.kernel.org/all/20250114-perf_syscall_arch_runtime-v1-1-5b304e408e11@rivosinc.com/ > > I agree it is similar and progress, I think I prefer my changes :-) > > Anything in the arch directories is only going to be built given a > Makefile SRCARCH: > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/Build?h=perf-tools-next#n2 > which means if we want the system call tables for say ARM on x86-64 we > need to reinvent the build - this isn't a problem today but perhaps > something to consider if we wanted a cross-architecture perf trace > record. I'd like to do away entirely with the arch directory for this > kind of reason. For example, if we run perf on a user space emulator > (say a testing set up) and it sees AMD data fabric PMUs, logic for > those PMUs currently lives in the arch directory for x86 and likely > wasn't even built into the perf tool. We should be able to identify > PMUs and act accordingly based on their names. In the case of the > overloaded "cpu" PMU we can identify it with the CPUID. The less code > in arch and the fewer architecture ifdefs, the better things should > work cross-architecture, in testing scenarios, etc. We also win by > getting code coverage without, for example, having to build and test > on csky or super H. > > Your changes are using the string for the architecture name. I think > that string is something we should get rid of: > https://lore.kernel.org/lkml/CAP-5=fX2BtFzhGLCSqO1QszqfX=HT8RrTdG_5Ttp5Gw4seSstA@mail.gmail.com/ > but there's no reason we couldn't clean that up on top of your change. Yes that part is not ideal. > > The build with lots of separate Build/Makefiles is something of a pain > to port/maintain in a bazel version of the build I maintain at Google > (happy to open source if anyone cares). In these changes you just run > a big script and get a big header file out the end, which largely > matches the other perf trace beauty things we build. Fair enough! I wanted to mimic how the kernel was building the syscall tables as closely as possible, it's convenient to have cohesion. What is the usecase for the bazel version? - Charlie > > Thanks, > Ian > > > > > > > The patches generating a syscall(_32|_64)?.h landed. These changes > > > take your changes and make it so that we just run the script once > > > building a header file for all architectures. On x86 we then have the > > > tables be guarded by ifdefs on i386 and x86-64. Then rather than the > > > table just matching the host architecture the ELF machine is used for > > > the executable running. > > > > > > Thanks, > > > Ian > > > > > > > - Charlie > > > > > > > > > > > > > > Ian Rogers (7): > > > > > perf syscalltble: Remove syscall_table.h > > > > > perf trace: Reorganize syscalls > > > > > perf syscalltbl: Remove struct syscalltbl > > > > > perf thread: Add support for reading the e_machine type for a thread > > > > > perf trace beauty: Add syscalltbl.sh generating all system call tables > > > > > perf syscalltbl: Use lookup table containing multiple architectures > > > > > perf build: Remove Makefile.syscalls > > > > > > > > > > tools/perf/Makefile.perf | 10 +- > > > > > tools/perf/arch/alpha/entry/syscalls/Kbuild | 2 - > > > > > .../alpha/entry/syscalls/Makefile.syscalls | 5 - > > > > > tools/perf/arch/alpha/include/syscall_table.h | 2 - > > > > > tools/perf/arch/arc/entry/syscalls/Kbuild | 2 - > > > > > .../arch/arc/entry/syscalls/Makefile.syscalls | 3 - > > > > > tools/perf/arch/arc/include/syscall_table.h | 2 - > > > > > tools/perf/arch/arm/entry/syscalls/Kbuild | 4 - > > > > > .../arch/arm/entry/syscalls/Makefile.syscalls | 2 - > > > > > tools/perf/arch/arm/include/syscall_table.h | 2 - > > > > > tools/perf/arch/arm64/entry/syscalls/Kbuild | 3 - > > > > > .../arm64/entry/syscalls/Makefile.syscalls | 6 - > > > > > tools/perf/arch/arm64/include/syscall_table.h | 8 - > > > > > tools/perf/arch/csky/entry/syscalls/Kbuild | 2 - > > > > > .../csky/entry/syscalls/Makefile.syscalls | 3 - > > > > > tools/perf/arch/csky/include/syscall_table.h | 2 - > > > > > .../perf/arch/loongarch/entry/syscalls/Kbuild | 2 - > > > > > .../entry/syscalls/Makefile.syscalls | 3 - > > > > > .../arch/loongarch/include/syscall_table.h | 2 - > > > > > tools/perf/arch/mips/entry/syscalls/Kbuild | 2 - > > > > > .../mips/entry/syscalls/Makefile.syscalls | 5 - > > > > > tools/perf/arch/mips/include/syscall_table.h | 2 - > > > > > tools/perf/arch/parisc/entry/syscalls/Kbuild | 3 - > > > > > .../parisc/entry/syscalls/Makefile.syscalls | 6 - > > > > > .../perf/arch/parisc/include/syscall_table.h | 8 - > > > > > tools/perf/arch/powerpc/entry/syscalls/Kbuild | 3 - > > > > > .../powerpc/entry/syscalls/Makefile.syscalls | 6 - > > > > > .../perf/arch/powerpc/include/syscall_table.h | 8 - > > > > > tools/perf/arch/riscv/entry/syscalls/Kbuild | 2 - > > > > > .../riscv/entry/syscalls/Makefile.syscalls | 4 - > > > > > tools/perf/arch/riscv/include/syscall_table.h | 8 - > > > > > tools/perf/arch/s390/entry/syscalls/Kbuild | 2 - > > > > > .../s390/entry/syscalls/Makefile.syscalls | 5 - > > > > > tools/perf/arch/s390/include/syscall_table.h | 2 - > > > > > tools/perf/arch/sh/entry/syscalls/Kbuild | 2 - > > > > > .../arch/sh/entry/syscalls/Makefile.syscalls | 4 - > > > > > tools/perf/arch/sh/include/syscall_table.h | 2 - > > > > > tools/perf/arch/sparc/entry/syscalls/Kbuild | 3 - > > > > > .../sparc/entry/syscalls/Makefile.syscalls | 5 - > > > > > tools/perf/arch/sparc/include/syscall_table.h | 8 - > > > > > tools/perf/arch/x86/entry/syscalls/Kbuild | 3 - > > > > > .../arch/x86/entry/syscalls/Makefile.syscalls | 6 - > > > > > tools/perf/arch/x86/include/syscall_table.h | 8 - > > > > > tools/perf/arch/xtensa/entry/syscalls/Kbuild | 2 - > > > > > .../xtensa/entry/syscalls/Makefile.syscalls | 4 - > > > > > .../perf/arch/xtensa/include/syscall_table.h | 2 - > > > > > tools/perf/builtin-trace.c | 275 +++++++++++------- > > > > > tools/perf/scripts/Makefile.syscalls | 61 ---- > > > > > tools/perf/scripts/syscalltbl.sh | 86 ------ > > > > > tools/perf/trace/beauty/syscalltbl.sh | 274 +++++++++++++++++ > > > > > tools/perf/util/syscalltbl.c | 142 ++++----- > > > > > tools/perf/util/syscalltbl.h | 22 +- > > > > > tools/perf/util/thread.c | 50 ++++ > > > > > tools/perf/util/thread.h | 14 +- > > > > > 54 files changed, 598 insertions(+), 506 deletions(-) > > > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/alpha/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/arc/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/arm/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/arm64/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/csky/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/loongarch/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/mips/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/parisc/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/powerpc/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/riscv/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/s390/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/sh/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/sparc/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/x86/include/syscall_table.h > > > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Kbuild > > > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Makefile.syscalls > > > > > delete mode 100644 tools/perf/arch/xtensa/include/syscall_table.h > > > > > delete mode 100644 tools/perf/scripts/Makefile.syscalls > > > > > delete mode 100755 tools/perf/scripts/syscalltbl.sh > > > > > create mode 100755 tools/perf/trace/beauty/syscalltbl.sh > > > > > > > > > > -- > > > > > 2.48.1.362.g079036d154-goog > > > > >
On Mon, Feb 3, 2025 at 12:06 PM Charlie Jenkins <charlie@rivosinc.com> wrote: > > On Mon, Feb 03, 2025 at 11:39:01AM -0800, Ian Rogers wrote: > > On Mon, Feb 3, 2025 at 11:15 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > > > On Mon, Feb 03, 2025 at 11:10:49AM -0800, Ian Rogers wrote: > > > > On Mon, Feb 3, 2025 at 11:02 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > > > > > > > On Fri, Jan 31, 2025 at 11:14:48PM -0800, Ian Rogers wrote: > > > > > > This work builds on the clean up of system call tables and removal of > > > > > > libaudit by Charlie Jenkins <charlie@rivosinc.com>. > > > > > > > > > > > > The system call table in perf trace is used to map system call numbers > > > > > > to names and vice versa. Prior to these changes, a single table > > > > > > matching the perf binary's build was present. The table would be > > > > > > incorrect if tracing say a 32-bit binary from a 64-bit version of > > > > > > perf, the names and numbers wouldn't match. > > > > > > > > > > > > Change the build so that a single system call file is built and the > > > > > > potentially multiple tables are identifiable from the ELF machine type > > > > > > of the process being examined. To determine the ELF machine type, the > > > > > > executable's header is read from /proc/pid/exe with fallbacks to using > > > > > > the perf's binary type when unknown. > > > > > > > > > > > > Remove some runtime types used by the system call tables and make > > > > > > equivalents generated at build time. > > > > > > > > > > Our approaches are very different but I sent out a patch to do this a > > > > > couple of weeks ago [1]. > > > > > > > > > > Did you look at that and decide you didn't like the approach? > > > > > > > > Missing link? > > > > > > Whoops here is the link: > > > > > > https://lore.kernel.org/all/20250114-perf_syscall_arch_runtime-v1-1-5b304e408e11@rivosinc.com/ > > > > I agree it is similar and progress, I think I prefer my changes :-) > > > > Anything in the arch directories is only going to be built given a > > Makefile SRCARCH: > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/Build?h=perf-tools-next#n2 > > which means if we want the system call tables for say ARM on x86-64 we > > need to reinvent the build - this isn't a problem today but perhaps > > something to consider if we wanted a cross-architecture perf trace > > record. I'd like to do away entirely with the arch directory for this > > kind of reason. For example, if we run perf on a user space emulator > > (say a testing set up) and it sees AMD data fabric PMUs, logic for > > those PMUs currently lives in the arch directory for x86 and likely > > wasn't even built into the perf tool. We should be able to identify > > PMUs and act accordingly based on their names. In the case of the > > overloaded "cpu" PMU we can identify it with the CPUID. The less code > > in arch and the fewer architecture ifdefs, the better things should > > work cross-architecture, in testing scenarios, etc. We also win by > > getting code coverage without, for example, having to build and test > > on csky or super H. > > > > Your changes are using the string for the architecture name. I think > > that string is something we should get rid of: > > https://lore.kernel.org/lkml/CAP-5=fX2BtFzhGLCSqO1QszqfX=HT8RrTdG_5Ttp5Gw4seSstA@mail.gmail.com/ > > but there's no reason we couldn't clean that up on top of your change. > > Yes that part is not ideal. > > > > > The build with lots of separate Build/Makefiles is something of a pain > > to port/maintain in a bazel version of the build I maintain at Google > > (happy to open source if anyone cares). In these changes you just run > > a big script and get a big header file out the end, which largely > > matches the other perf trace beauty things we build. > > Fair enough! I wanted to mimic how the kernel was building the syscall > tables as closely as possible, it's convenient to have cohesion. I think it makes sense in the kernel, as the built binary doesn't have cross-platform concerns. This is probably also the reason why the perf tool has an arch directory. Let me know what you think is the right direction for the perf tool syscall table code. > What is the usecase for the bazel version? I'm sure there's more info in: https://bazel.build/ but the main thing is having hermetic and reproducible builds. The main problem with open sourcing would be dealing with external repositories, test set up, etc. Tbh, hermetic builds are a challenge as things like lex and yacc will vary the C code they generate based on the platform they run on. Bazel has plans to change the world so maybe this could be more of a thing in the future. Fwiw, I did push changes into the perf tree so that man page building was reproducible: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/tools/perf/Documentation?h=perf-tools-next&id=d586ac10ce56b2381b8e1d8ed74660c1b2b8ab0d The man pages used to reflect the date/time they were built, but now reflect the date the of the commit of the file being built: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/Documentation/Makefile?h=perf-tools-next#n255 Thanks, Ian > - Charlie > > > > > Thanks, > > Ian > > > > > > > > > > The patches generating a syscall(_32|_64)?.h landed. These changes > > > > take your changes and make it so that we just run the script once > > > > building a header file for all architectures. On x86 we then have the > > > > tables be guarded by ifdefs on i386 and x86-64. Then rather than the > > > > table just matching the host architecture the ELF machine is used for > > > > the executable running. > > > > > > > > Thanks, > > > > Ian > > > > > > > > > - Charlie > > > > > > > > > > > > > > > > > Ian Rogers (7): > > > > > > perf syscalltble: Remove syscall_table.h > > > > > > perf trace: Reorganize syscalls > > > > > > perf syscalltbl: Remove struct syscalltbl > > > > > > perf thread: Add support for reading the e_machine type for a thread > > > > > > perf trace beauty: Add syscalltbl.sh generating all system call tables > > > > > > perf syscalltbl: Use lookup table containing multiple architectures > > > > > > perf build: Remove Makefile.syscalls > > > > > > > > > > > > tools/perf/Makefile.perf | 10 +- > > > > > > tools/perf/arch/alpha/entry/syscalls/Kbuild | 2 - > > > > > > .../alpha/entry/syscalls/Makefile.syscalls | 5 - > > > > > > tools/perf/arch/alpha/include/syscall_table.h | 2 - > > > > > > tools/perf/arch/arc/entry/syscalls/Kbuild | 2 - > > > > > > .../arch/arc/entry/syscalls/Makefile.syscalls | 3 - > > > > > > tools/perf/arch/arc/include/syscall_table.h | 2 - > > > > > > tools/perf/arch/arm/entry/syscalls/Kbuild | 4 - > > > > > > .../arch/arm/entry/syscalls/Makefile.syscalls | 2 - > > > > > > tools/perf/arch/arm/include/syscall_table.h | 2 - > > > > > > tools/perf/arch/arm64/entry/syscalls/Kbuild | 3 - > > > > > > .../arm64/entry/syscalls/Makefile.syscalls | 6 - > > > > > > tools/perf/arch/arm64/include/syscall_table.h | 8 - > > > > > > tools/perf/arch/csky/entry/syscalls/Kbuild | 2 - > > > > > > .../csky/entry/syscalls/Makefile.syscalls | 3 - > > > > > > tools/perf/arch/csky/include/syscall_table.h | 2 - > > > > > > .../perf/arch/loongarch/entry/syscalls/Kbuild | 2 - > > > > > > .../entry/syscalls/Makefile.syscalls | 3 - > > > > > > .../arch/loongarch/include/syscall_table.h | 2 - > > > > > > tools/perf/arch/mips/entry/syscalls/Kbuild | 2 - > > > > > > .../mips/entry/syscalls/Makefile.syscalls | 5 - > > > > > > tools/perf/arch/mips/include/syscall_table.h | 2 - > > > > > > tools/perf/arch/parisc/entry/syscalls/Kbuild | 3 - > > > > > > .../parisc/entry/syscalls/Makefile.syscalls | 6 - > > > > > > .../perf/arch/parisc/include/syscall_table.h | 8 - > > > > > > tools/perf/arch/powerpc/entry/syscalls/Kbuild | 3 - > > > > > > .../powerpc/entry/syscalls/Makefile.syscalls | 6 - > > > > > > .../perf/arch/powerpc/include/syscall_table.h | 8 - > > > > > > tools/perf/arch/riscv/entry/syscalls/Kbuild | 2 - > > > > > > .../riscv/entry/syscalls/Makefile.syscalls | 4 - > > > > > > tools/perf/arch/riscv/include/syscall_table.h | 8 - > > > > > > tools/perf/arch/s390/entry/syscalls/Kbuild | 2 - > > > > > > .../s390/entry/syscalls/Makefile.syscalls | 5 - > > > > > > tools/perf/arch/s390/include/syscall_table.h | 2 - > > > > > > tools/perf/arch/sh/entry/syscalls/Kbuild | 2 - > > > > > > .../arch/sh/entry/syscalls/Makefile.syscalls | 4 - > > > > > > tools/perf/arch/sh/include/syscall_table.h | 2 - > > > > > > tools/perf/arch/sparc/entry/syscalls/Kbuild | 3 - > > > > > > .../sparc/entry/syscalls/Makefile.syscalls | 5 - > > > > > > tools/perf/arch/sparc/include/syscall_table.h | 8 - > > > > > > tools/perf/arch/x86/entry/syscalls/Kbuild | 3 - > > > > > > .../arch/x86/entry/syscalls/Makefile.syscalls | 6 - > > > > > > tools/perf/arch/x86/include/syscall_table.h | 8 - > > > > > > tools/perf/arch/xtensa/entry/syscalls/Kbuild | 2 - > > > > > > .../xtensa/entry/syscalls/Makefile.syscalls | 4 - > > > > > > .../perf/arch/xtensa/include/syscall_table.h | 2 - > > > > > > tools/perf/builtin-trace.c | 275 +++++++++++------- > > > > > > tools/perf/scripts/Makefile.syscalls | 61 ---- > > > > > > tools/perf/scripts/syscalltbl.sh | 86 ------ > > > > > > tools/perf/trace/beauty/syscalltbl.sh | 274 +++++++++++++++++ > > > > > > tools/perf/util/syscalltbl.c | 142 ++++----- > > > > > > tools/perf/util/syscalltbl.h | 22 +- > > > > > > tools/perf/util/thread.c | 50 ++++ > > > > > > tools/perf/util/thread.h | 14 +- > > > > > > 54 files changed, 598 insertions(+), 506 deletions(-) > > > > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/alpha/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/arc/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/arm/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/arm64/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/csky/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/loongarch/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/mips/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/parisc/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/powerpc/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/riscv/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/s390/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/sh/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/sparc/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/x86/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Kbuild > > > > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Makefile.syscalls > > > > > > delete mode 100644 tools/perf/arch/xtensa/include/syscall_table.h > > > > > > delete mode 100644 tools/perf/scripts/Makefile.syscalls > > > > > > delete mode 100755 tools/perf/scripts/syscalltbl.sh > > > > > > create mode 100755 tools/perf/trace/beauty/syscalltbl.sh > > > > > > > > > > > > -- > > > > > > 2.48.1.362.g079036d154-goog > > > > > >
On Mon, Feb 03, 2025 at 12:54:59PM -0800, Ian Rogers wrote: > On Mon, Feb 3, 2025 at 12:06 PM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > On Mon, Feb 03, 2025 at 11:39:01AM -0800, Ian Rogers wrote: > > > On Mon, Feb 3, 2025 at 11:15 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > > > > > On Mon, Feb 03, 2025 at 11:10:49AM -0800, Ian Rogers wrote: > > > > > On Mon, Feb 3, 2025 at 11:02 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > > > > > > > > > On Fri, Jan 31, 2025 at 11:14:48PM -0800, Ian Rogers wrote: > > > > > > > This work builds on the clean up of system call tables and removal of > > > > > > > libaudit by Charlie Jenkins <charlie@rivosinc.com>. > > > > > > > > > > > > > > The system call table in perf trace is used to map system call numbers > > > > > > > to names and vice versa. Prior to these changes, a single table > > > > > > > matching the perf binary's build was present. The table would be > > > > > > > incorrect if tracing say a 32-bit binary from a 64-bit version of > > > > > > > perf, the names and numbers wouldn't match. > > > > > > > > > > > > > > Change the build so that a single system call file is built and the > > > > > > > potentially multiple tables are identifiable from the ELF machine type > > > > > > > of the process being examined. To determine the ELF machine type, the > > > > > > > executable's header is read from /proc/pid/exe with fallbacks to using > > > > > > > the perf's binary type when unknown. > > > > > > > > > > > > > > Remove some runtime types used by the system call tables and make > > > > > > > equivalents generated at build time. > > > > > > > > > > > > Our approaches are very different but I sent out a patch to do this a > > > > > > couple of weeks ago [1]. > > > > > > > > > > > > Did you look at that and decide you didn't like the approach? > > > > > > > > > > Missing link? > > > > > > > > Whoops here is the link: > > > > > > > > https://lore.kernel.org/all/20250114-perf_syscall_arch_runtime-v1-1-5b304e408e11@rivosinc.com/ > > > > > > I agree it is similar and progress, I think I prefer my changes :-) > > > > > > Anything in the arch directories is only going to be built given a > > > Makefile SRCARCH: > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/Build?h=perf-tools-next#n2 > > > which means if we want the system call tables for say ARM on x86-64 we > > > need to reinvent the build - this isn't a problem today but perhaps > > > something to consider if we wanted a cross-architecture perf trace > > > record. I'd like to do away entirely with the arch directory for this > > > kind of reason. For example, if we run perf on a user space emulator > > > (say a testing set up) and it sees AMD data fabric PMUs, logic for > > > those PMUs currently lives in the arch directory for x86 and likely > > > wasn't even built into the perf tool. We should be able to identify > > > PMUs and act accordingly based on their names. In the case of the > > > overloaded "cpu" PMU we can identify it with the CPUID. The less code > > > in arch and the fewer architecture ifdefs, the better things should > > > work cross-architecture, in testing scenarios, etc. We also win by > > > getting code coverage without, for example, having to build and test > > > on csky or super H. > > > > > > Your changes are using the string for the architecture name. I think > > > that string is something we should get rid of: > > > https://lore.kernel.org/lkml/CAP-5=fX2BtFzhGLCSqO1QszqfX=HT8RrTdG_5Ttp5Gw4seSstA@mail.gmail.com/ > > > but there's no reason we couldn't clean that up on top of your change. > > > > Yes that part is not ideal. > > > > > > > > The build with lots of separate Build/Makefiles is something of a pain > > > to port/maintain in a bazel version of the build I maintain at Google > > > (happy to open source if anyone cares). In these changes you just run > > > a big script and get a big header file out the end, which largely > > > matches the other perf trace beauty things we build. > > > > Fair enough! I wanted to mimic how the kernel was building the syscall > > tables as closely as possible, it's convenient to have cohesion. > > I think it makes sense in the kernel, as the built binary doesn't have > cross-platform concerns. This is probably also the reason why the perf > tool has an arch directory. Let me know what you think is the right > direction for the perf tool syscall table code. I am hesitant about moving all of the arch-specific syscall generation flags into a single file. There is currently a really clean separation between the architectures and it's possible to generate all of the syscall tables for all of the architecutures based on the paths to the syscall table. What causes the arch directory to be a pain for Bazel? I guess I am mostly confused why it makes sense to change the kernel Makefiles in order to accomidate a rewrite of the build system in Bazel that isn't planned to be used upstream. - Charlie > > > What is the usecase for the bazel version? > > I'm sure there's more info in: > https://bazel.build/ > but the main thing is having hermetic and reproducible builds. The > main problem with open sourcing would be dealing with external > repositories, test set up, etc. Tbh, hermetic builds are a challenge > as things like lex and yacc will vary the C code they generate based > on the platform they run on. Bazel has plans to change the world so > maybe this could be more of a thing in the future. > > Fwiw, I did push changes into the perf tree so that man page building > was reproducible: > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/tools/perf/Documentation?h=perf-tools-next&id=d586ac10ce56b2381b8e1d8ed74660c1b2b8ab0d > The man pages used to reflect the date/time they were built, but now > reflect the date the of the commit of the file being built: > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/Documentation/Makefile?h=perf-tools-next#n255 > > Thanks, > Ian > > > - Charlie > > > > > > > > Thanks, > > > Ian > > > > > > > > > > > > > The patches generating a syscall(_32|_64)?.h landed. These changes > > > > > take your changes and make it so that we just run the script once > > > > > building a header file for all architectures. On x86 we then have the > > > > > tables be guarded by ifdefs on i386 and x86-64. Then rather than the > > > > > table just matching the host architecture the ELF machine is used for > > > > > the executable running. > > > > > > > > > > Thanks, > > > > > Ian > > > > > > > > > > > - Charlie > > > > > > > > > > > > > > > > > > > > Ian Rogers (7): > > > > > > > perf syscalltble: Remove syscall_table.h > > > > > > > perf trace: Reorganize syscalls > > > > > > > perf syscalltbl: Remove struct syscalltbl > > > > > > > perf thread: Add support for reading the e_machine type for a thread > > > > > > > perf trace beauty: Add syscalltbl.sh generating all system call tables > > > > > > > perf syscalltbl: Use lookup table containing multiple architectures > > > > > > > perf build: Remove Makefile.syscalls > > > > > > > > > > > > > > tools/perf/Makefile.perf | 10 +- > > > > > > > tools/perf/arch/alpha/entry/syscalls/Kbuild | 2 - > > > > > > > .../alpha/entry/syscalls/Makefile.syscalls | 5 - > > > > > > > tools/perf/arch/alpha/include/syscall_table.h | 2 - > > > > > > > tools/perf/arch/arc/entry/syscalls/Kbuild | 2 - > > > > > > > .../arch/arc/entry/syscalls/Makefile.syscalls | 3 - > > > > > > > tools/perf/arch/arc/include/syscall_table.h | 2 - > > > > > > > tools/perf/arch/arm/entry/syscalls/Kbuild | 4 - > > > > > > > .../arch/arm/entry/syscalls/Makefile.syscalls | 2 - > > > > > > > tools/perf/arch/arm/include/syscall_table.h | 2 - > > > > > > > tools/perf/arch/arm64/entry/syscalls/Kbuild | 3 - > > > > > > > .../arm64/entry/syscalls/Makefile.syscalls | 6 - > > > > > > > tools/perf/arch/arm64/include/syscall_table.h | 8 - > > > > > > > tools/perf/arch/csky/entry/syscalls/Kbuild | 2 - > > > > > > > .../csky/entry/syscalls/Makefile.syscalls | 3 - > > > > > > > tools/perf/arch/csky/include/syscall_table.h | 2 - > > > > > > > .../perf/arch/loongarch/entry/syscalls/Kbuild | 2 - > > > > > > > .../entry/syscalls/Makefile.syscalls | 3 - > > > > > > > .../arch/loongarch/include/syscall_table.h | 2 - > > > > > > > tools/perf/arch/mips/entry/syscalls/Kbuild | 2 - > > > > > > > .../mips/entry/syscalls/Makefile.syscalls | 5 - > > > > > > > tools/perf/arch/mips/include/syscall_table.h | 2 - > > > > > > > tools/perf/arch/parisc/entry/syscalls/Kbuild | 3 - > > > > > > > .../parisc/entry/syscalls/Makefile.syscalls | 6 - > > > > > > > .../perf/arch/parisc/include/syscall_table.h | 8 - > > > > > > > tools/perf/arch/powerpc/entry/syscalls/Kbuild | 3 - > > > > > > > .../powerpc/entry/syscalls/Makefile.syscalls | 6 - > > > > > > > .../perf/arch/powerpc/include/syscall_table.h | 8 - > > > > > > > tools/perf/arch/riscv/entry/syscalls/Kbuild | 2 - > > > > > > > .../riscv/entry/syscalls/Makefile.syscalls | 4 - > > > > > > > tools/perf/arch/riscv/include/syscall_table.h | 8 - > > > > > > > tools/perf/arch/s390/entry/syscalls/Kbuild | 2 - > > > > > > > .../s390/entry/syscalls/Makefile.syscalls | 5 - > > > > > > > tools/perf/arch/s390/include/syscall_table.h | 2 - > > > > > > > tools/perf/arch/sh/entry/syscalls/Kbuild | 2 - > > > > > > > .../arch/sh/entry/syscalls/Makefile.syscalls | 4 - > > > > > > > tools/perf/arch/sh/include/syscall_table.h | 2 - > > > > > > > tools/perf/arch/sparc/entry/syscalls/Kbuild | 3 - > > > > > > > .../sparc/entry/syscalls/Makefile.syscalls | 5 - > > > > > > > tools/perf/arch/sparc/include/syscall_table.h | 8 - > > > > > > > tools/perf/arch/x86/entry/syscalls/Kbuild | 3 - > > > > > > > .../arch/x86/entry/syscalls/Makefile.syscalls | 6 - > > > > > > > tools/perf/arch/x86/include/syscall_table.h | 8 - > > > > > > > tools/perf/arch/xtensa/entry/syscalls/Kbuild | 2 - > > > > > > > .../xtensa/entry/syscalls/Makefile.syscalls | 4 - > > > > > > > .../perf/arch/xtensa/include/syscall_table.h | 2 - > > > > > > > tools/perf/builtin-trace.c | 275 +++++++++++------- > > > > > > > tools/perf/scripts/Makefile.syscalls | 61 ---- > > > > > > > tools/perf/scripts/syscalltbl.sh | 86 ------ > > > > > > > tools/perf/trace/beauty/syscalltbl.sh | 274 +++++++++++++++++ > > > > > > > tools/perf/util/syscalltbl.c | 142 ++++----- > > > > > > > tools/perf/util/syscalltbl.h | 22 +- > > > > > > > tools/perf/util/thread.c | 50 ++++ > > > > > > > tools/perf/util/thread.h | 14 +- > > > > > > > 54 files changed, 598 insertions(+), 506 deletions(-) > > > > > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/alpha/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/alpha/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/arc/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/arc/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/arm/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/arm/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/arm64/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/csky/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/csky/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/loongarch/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/loongarch/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/mips/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/mips/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/parisc/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/parisc/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/powerpc/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/powerpc/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/riscv/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/riscv/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/s390/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/s390/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/sh/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/sh/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/sparc/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/sparc/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/x86/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/x86/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Kbuild > > > > > > > delete mode 100644 tools/perf/arch/xtensa/entry/syscalls/Makefile.syscalls > > > > > > > delete mode 100644 tools/perf/arch/xtensa/include/syscall_table.h > > > > > > > delete mode 100644 tools/perf/scripts/Makefile.syscalls > > > > > > > delete mode 100755 tools/perf/scripts/syscalltbl.sh > > > > > > > create mode 100755 tools/perf/trace/beauty/syscalltbl.sh > > > > > > > > > > > > > > -- > > > > > > > 2.48.1.362.g079036d154-goog > > > > > > >
On Mon, Feb 3, 2025 at 3:02 PM Charlie Jenkins <charlie@rivosinc.com> wrote: > On Mon, Feb 03, 2025 at 12:54:59PM -0800, Ian Rogers wrote: [snip] > > I think it makes sense in the kernel, as the built binary doesn't have > > cross-platform concerns. This is probably also the reason why the perf > > tool has an arch directory. Let me know what you think is the right > > direction for the perf tool syscall table code. > > I am hesitant about moving all of the arch-specific syscall generation > flags into a single file. In these changes I had a single file to build up a mapping from ELF machine to syscall table in an array and I wanted to keep the logic to build the array alongside the logic to build up the components of the array - so the ifdefs were visually the same. As the scope is a single file and the variables are static, this can give useful C compiler "unused definition" warnings. You can trick the linker to give similar warnings at the scope of a file, so this isn't a deal breaker. > There is currently a really clean separation > between the architectures and it's possible to generate all of the > syscall tables for all of the architecutures based on the paths to the > syscall table. This doesn't happen currently as the build of the arch directory is to add in $(SRCARCH) only. So tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls will only be included into the build if SRCARCH==arm64. As I've said I'm against the idea of the arch directory as it nearly always causes cross platform problems - not an issue in a Linux kernel build. We recently eliminated dwarf-regs for this reason (and hopefully fixing up cross platform disassembly issues as a consequence): https://lore.kernel.org/all/20241108234606.429459-1-irogers@google.com/ We could have the syscall table logic not under arch and generate multiple files, but we'd be adding extra logic to pull things apart to then pull things back together again, which feels like unnecessary complexity. It seems in your changes the Kbuild and Makefile.syscalls are running in the arch directory - this feels like a lot of peculiar and separated build logic for just iterating over a bunch of arch directory names and calling a shell function on them - albeit with some arch specific parameters. There's also an extra C helper executable in your code. I kind of like that I get a single header that is the same across all architectures and with no more build system requirements than to support ifdefs - in fact the ifdefs are just there to keep the code size down there is a #define to make them all have no effect. I hear your "clean separation" but I also think separation across files can make things harder to read, state is in >1 place. I've tried to cleanly separate within the script. I do think there is some tech debt in both changes. My: ``` #if defined(ALL_SYSCALLTBL) || defined(__riscv) #if __BITS_PER_LONG != 64 EOF build_tables "$tools_dir/scripts/syscall.tbl" "$outfile" common,32,riscv,memfd_secret EM_RISCV echo "#else" >> "$outfile" build_tables "$tools_dir/scripts/syscall.tbl" "$outfile" common,64,riscv,rlimit,memfd_secret EM_RISC V cat >> "$outfile" <<EOF #endif //__BITS_PER_LONG != 64 ``` means the perf binary word size determines the syscall table support. This is because the e_machine in the ELF header isn't unique in these two cases and having both tables would have no effect. You've moved this into the env arch name handling, but I think having >1 way to encode a binary type is suboptimal. There are some ELF flag ABI bits that resolve disassembler things on csky, so perhaps a resolution is to pass ELF flags along with the machine type. I'm not clear in your change how "32_riscv" is generated to solve the same problem. Imo, it'd kind of be nice not to introduce notions like "64_arm64" as we seem to be always mapping/normalizing/... these different notions and they feel inherently brittle. > What causes the arch directory to be a pain for Bazel? I > guess I am mostly confused why it makes sense to change the kernel > Makefiles in order to accomidate a rewrite of the build system in Bazel > that isn't planned to be used upstream. It's just software and so the issues are resolvable, ie I don't think bazel should be determining what happens upstream - it motivates me to some extent. For the bazel build I need to match the Makefile behavior with bits of script called genrule, the scope and quantity of these increase with the arch directory model, extra executables to have in the build etc. I also imagine cross platform stuff will add complexity, like mapping blaze's notions of machine type to those introduced in your change. It is all a lot of stuff and I think what's in these changes keeps things about as simple as they can be. It'd be nice to integrate the best features of both changes and I think some of the e_machine stuff here can be added onto your change to get the i386/x86-64 case to work. I'm not sold on the complexity of the build in your changes though, multiple files, Kbuild and Makefile.syscalls, the arch directory not optionally being built, helper executables. Ultimately it is the same shell logic in both changes, and your previous work laid out all the ground work for this (I'm very grateful for it :-) ). Thanks, Ian
On Mon, Feb 03, 2025 at 05:58:29PM -0800, Ian Rogers wrote: > On Mon, Feb 3, 2025 at 3:02 PM Charlie Jenkins <charlie@rivosinc.com> wrote: > > On Mon, Feb 03, 2025 at 12:54:59PM -0800, Ian Rogers wrote: > [snip] > > > I think it makes sense in the kernel, as the built binary doesn't have > > > cross-platform concerns. This is probably also the reason why the perf > > > tool has an arch directory. Let me know what you think is the right > > > direction for the perf tool syscall table code. > > > > I am hesitant about moving all of the arch-specific syscall generation > > flags into a single file. > > In these changes I had a single file to build up a mapping from ELF > machine to syscall table in an array and I wanted to keep the logic to > build the array alongside the logic to build up the components of the > array - so the ifdefs were visually the same. As the scope is a single > file and the variables are static, this can give useful C compiler > "unused definition" warnings. You can trick the linker to give similar > warnings at the scope of a file, so this isn't a deal breaker. > > > There is currently a really clean separation > > between the architectures and it's possible to generate all of the > > syscall tables for all of the architecutures based on the paths to the > > syscall table. > > This doesn't happen currently as the build of the arch directory is to > add in $(SRCARCH) only. So > tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls will only be > included into the build if SRCARCH==arm64. As I've said I'm against > the idea of the arch directory as it nearly always causes cross > platform problems - not an issue in a Linux kernel build. We recently > eliminated dwarf-regs for this reason (and hopefully fixing up cross > platform disassembly issues as a consequence): > https://lore.kernel.org/all/20241108234606.429459-1-irogers@google.com/ > We could have the syscall table logic not under arch and generate > multiple files, but we'd be adding extra logic to pull things apart to > then pull things back together again, which feels like unnecessary > complexity. > > It seems in your changes the Kbuild and Makefile.syscalls are running > in the arch directory - this feels like a lot of peculiar and > separated build logic for just iterating over a bunch of arch > directory names and calling a shell function on them - albeit with > some arch specific parameters. There's also an extra C helper > executable in your code. Yes, I can understand why you be opposed to that and I don't have a good counterargument. > I kind of like that I get a single header > that is the same across all architectures and with no more build > system requirements than to support ifdefs - in fact the ifdefs are > just there to keep the code size down there is a #define to make them > all have no effect. I hear your "clean separation" but I also think > separation across files can make things harder to read, state is in >1 > place. I've tried to cleanly separate within the script. > > I do think there is some tech debt in both changes. My: > ``` > #if defined(ALL_SYSCALLTBL) || defined(__riscv) > #if __BITS_PER_LONG != 64 > EOF > build_tables "$tools_dir/scripts/syscall.tbl" "$outfile" > common,32,riscv,memfd_secret EM_RISCV > echo "#else" >> "$outfile" > build_tables "$tools_dir/scripts/syscall.tbl" "$outfile" > common,64,riscv,rlimit,memfd_secret EM_RISC > V > cat >> "$outfile" <<EOF > #endif //__BITS_PER_LONG != 64 > ``` > means the perf binary word size determines the syscall table support. > This is because the e_machine in the ELF header isn't unique in these > two cases and having both tables would have no effect. You've moved > this into the env arch name handling, but I think having >1 way to > encode a binary type is suboptimal. There are some ELF flag ABI bits > that resolve disassembler things on csky, so perhaps a resolution is > to pass ELF flags along with the machine type. I'm not clear in your > change how "32_riscv" is generated to solve the same problem. Imo, > it'd kind of be nice not to introduce notions like "64_arm64" as we > seem to be always mapping/normalizing/... these different notions and > they feel inherently brittle. Maybe it is brittle? It could be mapped to e_machine, but that just might not be reasonable to work into the method from my patch. > > > What causes the arch directory to be a pain for Bazel? I > > guess I am mostly confused why it makes sense to change the kernel > > Makefiles in order to accomidate a rewrite of the build system in Bazel > > that isn't planned to be used upstream. > > It's just software and so the issues are resolvable, ie I don't think > bazel should be determining what happens upstream - it motivates me to > some extent. For the bazel build I need to match the Makefile behavior > with bits of script called genrule, the scope and quantity of these > increase with the arch directory model, extra executables to have in > the build etc. I also imagine cross platform stuff will add > complexity, like mapping blaze's notions of machine type to those > introduced in your change. It is all a lot of stuff and I think what's > in these changes keeps things about as simple as they can be. It'd be > nice to integrate the best features of both changes and I think some > of the e_machine stuff here can be added onto your change to get the > i386/x86-64 case to work. I'm not sold on the complexity of the build > in your changes though, multiple files, Kbuild and Makefile.syscalls, > the arch directory not optionally being built, helper executables. > Ultimately it is the same shell logic in both changes, and your > previous work laid out all the ground work for this (I'm very grateful > for it :-) ). Thanks for elaborating on this! I do wish we had more of this conversation on the original patch so we could have molded the original patch closer to what this one is doing. I know you did mention you would like to get rid of the arch directory as feedback on that patch but I hadn't realized that this was the direction you were hoping to take this. It does seem like the changes you have made in this patch will lead to a something that is more robust and simpler to maintain, so we can move forward with reviewing this patch and stop reviewing the one I wrote. - Charlie > > Thanks, > Ian
On Tue, Feb 4, 2025 at 11:05 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > On Mon, Feb 03, 2025 at 05:58:29PM -0800, Ian Rogers wrote: > > On Mon, Feb 3, 2025 at 3:02 PM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > On Mon, Feb 03, 2025 at 12:54:59PM -0800, Ian Rogers wrote: > > [snip] > > > > I think it makes sense in the kernel, as the built binary doesn't have > > > > cross-platform concerns. This is probably also the reason why the perf > > > > tool has an arch directory. Let me know what you think is the right > > > > direction for the perf tool syscall table code. > > > > > > I am hesitant about moving all of the arch-specific syscall generation > > > flags into a single file. > > > > In these changes I had a single file to build up a mapping from ELF > > machine to syscall table in an array and I wanted to keep the logic to > > build the array alongside the logic to build up the components of the > > array - so the ifdefs were visually the same. As the scope is a single > > file and the variables are static, this can give useful C compiler > > "unused definition" warnings. You can trick the linker to give similar > > warnings at the scope of a file, so this isn't a deal breaker. > > > > > There is currently a really clean separation > > > between the architectures and it's possible to generate all of the > > > syscall tables for all of the architecutures based on the paths to the > > > syscall table. > > > > This doesn't happen currently as the build of the arch directory is to > > add in $(SRCARCH) only. So > > tools/perf/arch/arm64/entry/syscalls/Makefile.syscalls will only be > > included into the build if SRCARCH==arm64. As I've said I'm against > > the idea of the arch directory as it nearly always causes cross > > platform problems - not an issue in a Linux kernel build. We recently > > eliminated dwarf-regs for this reason (and hopefully fixing up cross > > platform disassembly issues as a consequence): > > https://lore.kernel.org/all/20241108234606.429459-1-irogers@google.com/ > > We could have the syscall table logic not under arch and generate > > multiple files, but we'd be adding extra logic to pull things apart to > > then pull things back together again, which feels like unnecessary > > complexity. > > > > It seems in your changes the Kbuild and Makefile.syscalls are running > > in the arch directory - this feels like a lot of peculiar and > > separated build logic for just iterating over a bunch of arch > > directory names and calling a shell function on them - albeit with > > some arch specific parameters. There's also an extra C helper > > executable in your code. > > Yes, I can understand why you be opposed to that and I don't have a good > counterargument. > > > I kind of like that I get a single header > > that is the same across all architectures and with no more build > > system requirements than to support ifdefs - in fact the ifdefs are > > just there to keep the code size down there is a #define to make them > > all have no effect. I hear your "clean separation" but I also think > > separation across files can make things harder to read, state is in >1 > > place. I've tried to cleanly separate within the script. > > > > I do think there is some tech debt in both changes. My: > > ``` > > #if defined(ALL_SYSCALLTBL) || defined(__riscv) > > #if __BITS_PER_LONG != 64 > > EOF > > build_tables "$tools_dir/scripts/syscall.tbl" "$outfile" > > common,32,riscv,memfd_secret EM_RISCV > > echo "#else" >> "$outfile" > > build_tables "$tools_dir/scripts/syscall.tbl" "$outfile" > > common,64,riscv,rlimit,memfd_secret EM_RISC > > V > > cat >> "$outfile" <<EOF > > #endif //__BITS_PER_LONG != 64 > > ``` > > means the perf binary word size determines the syscall table support. > > This is because the e_machine in the ELF header isn't unique in these > > two cases and having both tables would have no effect. You've moved > > this into the env arch name handling, but I think having >1 way to > > encode a binary type is suboptimal. There are some ELF flag ABI bits > > that resolve disassembler things on csky, so perhaps a resolution is > > to pass ELF flags along with the machine type. I'm not clear in your > > change how "32_riscv" is generated to solve the same problem. Imo, > > it'd kind of be nice not to introduce notions like "64_arm64" as we > > seem to be always mapping/normalizing/... these different notions and > > they feel inherently brittle. > > Maybe it is brittle? It could be mapped to e_machine, but that just > might not be reasonable to work into the method from my patch. > > > > > > What causes the arch directory to be a pain for Bazel? I > > > guess I am mostly confused why it makes sense to change the kernel > > > Makefiles in order to accomidate a rewrite of the build system in Bazel > > > that isn't planned to be used upstream. > > > > It's just software and so the issues are resolvable, ie I don't think > > bazel should be determining what happens upstream - it motivates me to > > some extent. For the bazel build I need to match the Makefile behavior > > with bits of script called genrule, the scope and quantity of these > > increase with the arch directory model, extra executables to have in > > the build etc. I also imagine cross platform stuff will add > > complexity, like mapping blaze's notions of machine type to those > > introduced in your change. It is all a lot of stuff and I think what's > > in these changes keeps things about as simple as they can be. It'd be > > nice to integrate the best features of both changes and I think some > > of the e_machine stuff here can be added onto your change to get the > > i386/x86-64 case to work. I'm not sold on the complexity of the build > > in your changes though, multiple files, Kbuild and Makefile.syscalls, > > the arch directory not optionally being built, helper executables. > > Ultimately it is the same shell logic in both changes, and your > > previous work laid out all the ground work for this (I'm very grateful > > for it :-) ). > > Thanks for elaborating on this! I do wish we had more of this > conversation on the original patch so we could have molded the original > patch closer to what this one is doing. I know you did mention you would > like to get rid of the arch directory as feedback on that patch but I > hadn't realized that this was the direction you were hoping to take > this. It does seem like the changes you have made in this patch will > lead to a something that is more robust and simpler to maintain, so we > can move forward with reviewing this patch and stop reviewing the one I > wrote. Thanks and sorry for work done that may not land - the maintainers may prefer the approach in your patch and my opinions matter little to them. If it is any consolation I wrote an entire Rust symbol demangler [1] that I don't expect to land :-) I also appreciate that you've been cleaning up the tools build infrastructure along with your changes, something there isn't enough of. Thanks, Ian [1] https://lore.kernel.org/lkml/20250129193037.573431-1-irogers@google.com/