Re: [PATCH v1 0/7] perf: Support multiple system call tables in the build

[Ian Rogers]

On Mon, Feb 3, 2025 at 3:02 PM Charlie Jenkins <charlie@xxxxxxxxxxxx> 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@xxxxxxxxxx/
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