On 15/05/2025 11:56, Alan Maguire wrote: > On 09/05/2025 19:40, Andrii Nakryiko wrote: >> On Thu, May 8, 2025 at 6:22 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote: >>> >>> When testing v1 of [1] we noticed that functions with 0-sized structs >>> as parameters were not part of BTF encoding; this was fixed in v2. >>> However we need to make sure we handle such zero-sized structs >>> correctly since they confound the calling convention expectations - >>> no registers are used for the empty struct so this has knock-on effects >>> for subsequent register-parameter matching. >> >> Do you have a list (or at least an example) of the function we are >> talking about, just curious to see what's that. >> >> The question I have is whether it's safe to assume that regardless of >> architecture we can assume that zero-sized struct has no effect on >> register allocation (which would seem logical, but is that true for >> all ABIs). >> > > I've been investigating this a bit, specifically in the context of s390 > where we saw the test failure. The actual kernel function where I first > observed the zero-sized struct in practice is > > static int __io_run_local_work(struct io_ring_ctx *ctx, io_tw_token_t > tw, int min_events, int max_events); > > In s390 DWARF, we see the following representation for it: > > <1><6f7f788>: Abbrev Number: 104 (DW_TAG_subprogram) > <6f7f789> DW_AT_name : (indirect string, offset: 0x2c47f5): > __io_run_local_work > <6f7f78d> DW_AT_decl_file : 1 > <6f7f78e> DW_AT_decl_line : 1301 > <6f7f790> DW_AT_decl_column : 12 > <6f7f791> DW_AT_prototyped : 1 > <6f7f791> DW_AT_type : <0x6f413a2> > <6f7f795> DW_AT_low_pc : 0x99c850 > <6f7f79d> DW_AT_high_pc : 0x2b2 > <6f7f7a5> DW_AT_frame_base : 1 byte block: 9c > (DW_OP_call_frame_cfa) > <6f7f7a7> DW_AT_GNU_all_call_sites: 1 > <6f7f7a7> DW_AT_sibling : <0x6f802e6> > <2><6f7f7ab>: Abbrev Number: 53 (DW_TAG_formal_parameter) > <6f7f7ac> DW_AT_name : ctx > <6f7f7b0> DW_AT_decl_file : 1 > <6f7f7b1> DW_AT_decl_line : 1301 > <6f7f7b3> DW_AT_decl_column : 52 > <6f7f7b4> DW_AT_type : <0x6f6882b> > <6f7f7b8> DW_AT_location : 0x2babcbe (location list) > <6f7f7bc> DW_AT_GNU_locviews: 0x2babcac > <2><6f7f7c0>: Abbrev Number: 135 (DW_TAG_formal_parameter) > <6f7f7c2> DW_AT_name : tw > <6f7f7c5> DW_AT_decl_file : 1 > <6f7f7c6> DW_AT_decl_line : 1301 > <6f7f7c8> DW_AT_decl_column : 71 > <6f7f7c9> DW_AT_type : <0x6f6833e> > <6f7f7cd> DW_AT_location : 2 byte block: 73 0 (DW_OP_breg3 > (r3): 0) > > > ..i.e. we are using the expected calling-convention register (r3) here > for the zero-sized struct parameter. > > Contrast this with x86_64 and aarch64, where regardless of -O level we > appear to use an offset from the frame ptr to reference the zero-sized > struct. As a result the next parameter after the zero-sized struct uses > the next available calling-convention register (%rdi if the zero-sized > struct is the first arg, %rsi if it was the second etc) that was unused > by the zero-sized struct parameter. > > I don't see anything in the ABI specs which covers this scenario > exactly; I suspect the 0-sized object handling in cases other than s390 > is just using the usual > register size aggregate object handling > (passing a large struct as a parameter), and in s390 it's not. > > So long story short, we may need to take an arch-specific approach here > unfortunately. Great that CI flagged this as an issue too! > > Alan > > I discussed this with Jose, and the gcc behaviour with zero-sized structs varies a bit between architectures. Given that complexity, my inclination would be to class functions with 0-sized struct parameters as having inconsistent representations. They can then be tackled by adding location info on a per-site basis later as part of the inline-related work. For now we would just not emit BTF for them, since without that site-specific analysis we can't be sure from function signature alone where parameters are stored. In practice this means leaving one function out of kernel BTF. So long story short, I think it might make sense to withdraw this series for now and see if we can tweak Tony's patch to class functions with 0-sized parameters as inconsistent as the v1 version did, meaning they don't get a BTF representation. Thanks! Alan > > >> BTW, while looking at patch #2, I noticed that >> btf_distill_func_proto() disallows functions returning >> struct-by-value, which seems overly aggressive, at least for structs >> of up to 8 bytes. So maybe if we can validate that both cases are not >> introducing any new quirks across all supported architectures, we can >> solve both limitations? >> >> P.S., oh, and s390x selftest (test_struct_args) isn't happy, please check. >> >> >>> >>> Patch 1 updates BPF_PROG2() to handle the zero-sized struct case. >>> Patch 2 makes 0-sized structs a special case, allowing them to exist >>> as parameter representations in BTF without failing verification. >>> Patch 3 is a selftest that ensures the parameters after the 0-sized >>> struct are represented correctly. >>> >>> [1] https://lore.kernel.org/dwarves/20250502070318.1561924-1-tony.ambardar@xxxxxxxxx/ >>> >>> Alan Maguire (3): >>> libbpf: update BPF_PROG2() to handle empty structs >>> bpf: allow 0-sized structs as function parameters >>> selftests/bpf: add 0-length struct testing to tracing_struct tests >>> >>> kernel/bpf/btf.c | 2 +- >>> tools/lib/bpf/bpf_tracing.h | 6 ++++-- >>> .../selftests/bpf/prog_tests/tracing_struct.c | 2 ++ >>> tools/testing/selftests/bpf/progs/tracing_struct.c | 11 +++++++++++ >>> tools/testing/selftests/bpf/test_kmods/bpf_testmod.c | 12 ++++++++++++ >>> 5 files changed, 30 insertions(+), 3 deletions(-) >>> >>> -- >>> 2.39.3 >>> > >
