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
