Message ID | 875xzhzm2o.fsf@oracle.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | BPF |
Headers | show |
Series | Anonymous struct types in parameter lists in BPF selftests | expand |
On Thu, 2024-01-25 at 12:31 +0100, Jose E. Marchesi wrote: [...] > However, btf_dump_test_case_syntax.c explicitly tests the dumping of a C > function like the above: > > * - `fn_ptr2_t`: function, taking anonymous struct as a first arg and pointer > * to a function, that takes int and returns int, as a second arg; returning > * a pointer to a const pointer to a char. Equivalent to: > * typedef struct { int a; } s_t; > * typedef int (*fn_t)(int); > * typedef char * const * (*fn_ptr2_t)(s_t, fn_t); > > the function being: > > typedef char * const * (*fn_ptr2_t)(struct { > int a; > }, int (*)(int)); > > which is not really equivalent to the above because one is an anonymous > struct type, the other is named, and also the scope issue described > above. > > That makes me wonder, since this is testing the C generation from BTF, > what motivated this particular test? Is there some particular code in > the kernel (or anywhere else) that uses anonymous struct types defined > in parameter lists? If so, how are these functions used? fwiw, I can't find any FUNC_PROTO in test kernel BTF that have anonymous struct or pointer to anonymous struct as their parameter.
On Thu, Jan 25, 2024 at 3:31 AM Jose E. Marchesi <jose.marchesi@oracle.com> wrote: > > > Hello. > > In C functions whose declarations/definitions use struct types or enum > types (or pointers to them) in the parameter list, the scope of such > defined types is limited to the parameter list, which makes the > functions basically un-callable with type-correct arguments. > > Therefore GCC has always emitted a warning when it finds such function > declarations, be them named: > > int f ( struct root { int i; } *arg) > { > return arg->i; > } > > foo.c:1:9: warning: 'struct root' declared inside parameter list > will not be visible outside of this definition or declaration > 1 | int f(struct root { int i; } *_) > | ^~~~~~~~~~~ > > or anonymous: > > int f ( struct { int i; } *arg) > { > return arg->i; > } > > foo.c:1:9: warning: anonymous struct declared inside parameter list > will not be visible outside of this definition or declaration > 1 | int f ( struct { int i; } *arg) > | ^~~~~~ > > This warning cannot be disabled. > > Clang, on the other side, emits the warning by default when the type is > no anonymous (this warning can be disabled with -Wno-visibility): > > int f ( struct root { int i; } *arg) > { > return arg->i; > } > > foo.c:1:18: warning: declaration of 'struct root' will not be visible > outside of this function [-Wvisibility] > int f ( struct root { int i; } *arg) > > But it doesn't emit any warning if the type is anonymous, which is > puzzling to some (see > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108189). > > Now, there are a few BPF selftests that contain function declarations > that get arguments of anonymous struct types defined inline: > > btf_dump_test_case_bitfields.c > btf_dump_test_case_namespacing.c > btf_dump_test_case_packing.c > btf_dump_test_case_padding.c > btf_dump_test_case_syntax.c > > The first four tests can easily be changed to no longer use anonymous > definitions of struct types in the formal arguments, since their purpose > (as far as I can see) is to test quirks related to struct fields and > other unrelated issue. This makes them buildable with GCC with -Werror. > See diff below. > > However, btf_dump_test_case_syntax.c explicitly tests the dumping of a C > function like the above: > > * - `fn_ptr2_t`: function, taking anonymous struct as a first arg and pointer > * to a function, that takes int and returns int, as a second arg; returning > * a pointer to a const pointer to a char. Equivalent to: > * typedef struct { int a; } s_t; > * typedef int (*fn_t)(int); > * typedef char * const * (*fn_ptr2_t)(s_t, fn_t); > > the function being: > > typedef char * const * (*fn_ptr2_t)(struct { > int a; > }, int (*)(int)); > > which is not really equivalent to the above because one is an anonymous > struct type, the other is named, and also the scope issue described > above. "Equivalent" in the conceptual sense to explain arcane C syntax. > > That makes me wonder, since this is testing the C generation from BTF, > what motivated this particular test? Is there some particular code in > the kernel (or anywhere else) that uses anonymous struct types defined > in parameter lists? If so, how are these functions used? I don't remember, but I'm not sure I'd be able to come up with such monstrosity by myself (but who knows...) I used vmlinux BTF and kept fixing and improving BTF dumper until I could dump everything in vmlinux BTF and make it compile without warnings (that's my recollection). So I suspect there is something in kernel that uses similar kind of declarations. > > I understand the code above is legal C code, even if questionable in > practice, so perhaps the right thing to do is to build these selftests > with -Wno-error, because the warnings are actually expected? Is it possible to have #pragma equivalent of -Wno-error? That probably would be best. But yeah, we can just add -Wno-error to btf_dump_test_case_syntax.c in Makefile, if it's just one file. > > diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c > index e01690618e1e..7ee9f6fcb8d9 100644 > --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c > +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c > @@ -82,11 +82,12 @@ struct bitfield_flushed { > long b: 16; > }; > > -int f(struct { > +struct root { > struct bitfields_only_mixed_types _1; > struct bitfield_mixed_with_others _2; > struct bitfield_flushed _3; > -} *_) > +}; > +int f(struct root *_) > { > return 0; > } Changes like this are fine, if they don't change the order of type outputs (i.e., if tests still succeed with these changes, I have no objections). > diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_namespacing.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_namespacing.c > index 92a4ad428710..0472183ed56d 100644 [...]
> On Thu, Jan 25, 2024 at 3:31 AM Jose E. Marchesi > <jose.marchesi@oracle.com> wrote: >> >> >> Hello. >> >> In C functions whose declarations/definitions use struct types or enum >> types (or pointers to them) in the parameter list, the scope of such >> defined types is limited to the parameter list, which makes the >> functions basically un-callable with type-correct arguments. >> >> Therefore GCC has always emitted a warning when it finds such function >> declarations, be them named: >> >> int f ( struct root { int i; } *arg) >> { >> return arg->i; >> } >> >> foo.c:1:9: warning: 'struct root' declared inside parameter list >> will not be visible outside of this definition or declaration >> 1 | int f(struct root { int i; } *_) >> | ^~~~~~~~~~~ >> >> or anonymous: >> >> int f ( struct { int i; } *arg) >> { >> return arg->i; >> } >> >> foo.c:1:9: warning: anonymous struct declared inside parameter list >> will not be visible outside of this definition or declaration >> 1 | int f ( struct { int i; } *arg) >> | ^~~~~~ >> >> This warning cannot be disabled. >> >> Clang, on the other side, emits the warning by default when the type is >> no anonymous (this warning can be disabled with -Wno-visibility): >> >> int f ( struct root { int i; } *arg) >> { >> return arg->i; >> } >> >> foo.c:1:18: warning: declaration of 'struct root' will not be visible >> outside of this function [-Wvisibility] >> int f ( struct root { int i; } *arg) >> >> But it doesn't emit any warning if the type is anonymous, which is >> puzzling to some (see >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108189). >> >> Now, there are a few BPF selftests that contain function declarations >> that get arguments of anonymous struct types defined inline: >> >> btf_dump_test_case_bitfields.c >> btf_dump_test_case_namespacing.c >> btf_dump_test_case_packing.c >> btf_dump_test_case_padding.c >> btf_dump_test_case_syntax.c >> >> The first four tests can easily be changed to no longer use anonymous >> definitions of struct types in the formal arguments, since their purpose >> (as far as I can see) is to test quirks related to struct fields and >> other unrelated issue. This makes them buildable with GCC with -Werror. >> See diff below. >> >> However, btf_dump_test_case_syntax.c explicitly tests the dumping of a C >> function like the above: >> >> * - `fn_ptr2_t`: function, taking anonymous struct as a first arg and pointer >> * to a function, that takes int and returns int, as a second arg; returning >> * a pointer to a const pointer to a char. Equivalent to: >> * typedef struct { int a; } s_t; >> * typedef int (*fn_t)(int); >> * typedef char * const * (*fn_ptr2_t)(s_t, fn_t); >> >> the function being: >> >> typedef char * const * (*fn_ptr2_t)(struct { >> int a; >> }, int (*)(int)); >> >> which is not really equivalent to the above because one is an anonymous >> struct type, the other is named, and also the scope issue described >> above. > > "Equivalent" in the conceptual sense to explain arcane C syntax. > >> >> That makes me wonder, since this is testing the C generation from BTF, >> what motivated this particular test? Is there some particular code in >> the kernel (or anywhere else) that uses anonymous struct types defined >> in parameter lists? If so, how are these functions used? > > I don't remember, but I'm not sure I'd be able to come up with such > monstrosity by myself (but who knows...) I used vmlinux BTF and kept > fixing and improving BTF dumper until I could dump everything in > vmlinux BTF and make it compile without warnings (that's my > recollection). So I suspect there is something in kernel that uses > similar kind of declarations. Yeah that's what I thought. Such construction may become to happen in headers because of some (perhaps unintended and unused) corner case of application of macros, or who knows. I wouldn't be comfortable removing the test case. >> >> I understand the code above is legal C code, even if questionable in >> practice, so perhaps the right thing to do is to build these selftests >> with -Wno-error, because the warnings are actually expected? > > Is it possible to have #pragma equivalent of -Wno-error? That probably > would be best. But yeah, we can just add -Wno-error to > btf_dump_test_case_syntax.c in Makefile, if it's just one file. Yes I will try with the pragma. >> >> diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c >> index e01690618e1e..7ee9f6fcb8d9 100644 >> --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c >> +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c >> @@ -82,11 +82,12 @@ struct bitfield_flushed { >> long b: 16; >> }; >> >> -int f(struct { >> +struct root { >> struct bitfields_only_mixed_types _1; >> struct bitfield_mixed_with_others _2; >> struct bitfield_flushed _3; >> -} *_) >> +}; >> +int f(struct root *_) >> { >> return 0; >> } > > Changes like this are fine, if they don't change the order of type > outputs (i.e., if tests still succeed with these changes, I have no > objections). Ok, I will prepare a patch for all this. Thank you! >> diff --git >> a/tools/testing/selftests/bpf/progs/btf_dump_test_case_namespacing.c >> b/tools/testing/selftests/bpf/progs/btf_dump_test_case_namespacing.c >> index 92a4ad428710..0472183ed56d 100644 > > [...]
diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c index e01690618e1e..7ee9f6fcb8d9 100644 --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_bitfields.c @@ -82,11 +82,12 @@ struct bitfield_flushed { long b: 16; }; -int f(struct { +struct root { struct bitfields_only_mixed_types _1; struct bitfield_mixed_with_others _2; struct bitfield_flushed _3; -} *_) +}; +int f(struct root *_) { return 0; } diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_namespacing.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_namespacing.c index 92a4ad428710..0472183ed56d 100644 --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_namespacing.c +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_namespacing.c @@ -51,7 +51,7 @@ typedef int Z; /*------ END-EXPECTED-OUTPUT ------ */ -int f(struct { +struct root { struct S _1; S _2; union U _3; @@ -67,7 +67,8 @@ int f(struct { X xx; Y yy; Z zz; -} *_) +}; +int f(struct root *_) { return 0; } diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c index 7998f27df7dd..8a83f049029f 100644 --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c @@ -134,7 +134,7 @@ struct outer_packed_struct { /* ------ END-EXPECTED-OUTPUT ------ */ -int f(struct { +struct root { struct packed_trailing_space _1; struct non_packed_trailing_space _2; struct packed_fields _3; @@ -147,7 +147,8 @@ int f(struct { struct usb_host_endpoint _10; struct outer_nonpacked_struct _11; struct outer_packed_struct _12; -} *_) +}; +int f(struct root *_) { return 0; } diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c index 79276fbe454a..2e03d1455c12 100644 --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_padding.c @@ -222,7 +222,7 @@ struct outer_mixed_but_unpacked { /* ------ END-EXPECTED-OUTPUT ------ */ -int f(struct { +struct root { struct padded_implicitly _1; struct padded_explicitly _2; struct padded_a_lot _3; @@ -243,7 +243,8 @@ int f(struct { struct ib_wc _201; struct acpi_object_method _202; struct outer_mixed_but_unpacked _203; -} *_) +}; +int f(struct root *_) { return 0; }