| Message ID | 20221221055856.2786043-1-james.hilliard1@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: move struct definitions out of function params | expand |
On 12/20, James Hilliard wrote: > Anonymous structs can't be declared inside function parameter > definitions in current c standards, however clang doesn't detect this > condition currently while GCC does. > Details: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108189 > Fixes errors like: > progs/btf_dump_test_case_bitfields.c:85:7: error: anonymous struct > declared inside parameter list will not be visible outside of this > definition or declaration [-Werror] > 85 | int f(struct { > | ^~~~~~ > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> Acked-by: Stanislav Fomichev <sdf@google.com> Looking at the referenced thread, seems like it requires at lest C23 and I doubt default clang is using that; so seems fine to be pedantic here and move the definitions out. > --- > .../bpf/progs/btf_dump_test_case_bitfields.c | 9 ++++-- > .../progs/btf_dump_test_case_namespacing.c | 10 ++++--- > .../bpf/progs/btf_dump_test_case_packing.c | 10 ++++--- > .../bpf/progs/btf_dump_test_case_padding.c | 10 ++++--- > .../bpf/progs/btf_dump_test_case_syntax.c | 30 +++++++++++++------ > 5 files changed, 46 insertions(+), 23 deletions(-) > 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..c75f6bd06a49 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,16 @@ struct bitfield_flushed { > long b: 16; > }; > -int f(struct { > +/* ----- START-EXPECTED-OUTPUT ----- */ > +struct root_struct { > struct bitfields_only_mixed_types _1; > struct bitfield_mixed_with_others _2; > struct bitfield_flushed _3; > -} *_) > +}; > + > +/* ------ END-EXPECTED-OUTPUT ------ */ > + > +int f(struct root_struct *_) > { > 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..d7cf2a8487c9 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 > @@ -49,9 +49,7 @@ typedef int Y; > typedef int Z; > -/*------ END-EXPECTED-OUTPUT ------ */ > - > -int f(struct { > +struct root_struct { > struct S _1; > S _2; > union U _3; > @@ -67,7 +65,11 @@ int f(struct { > X xx; > Y yy; > Z zz; > -} *_) > +}; > + > +/*------ END-EXPECTED-OUTPUT ------ */ > + > +int f(struct root_struct *_) > { > 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..e039ceb50c43 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 > @@ -132,9 +132,7 @@ struct outer_packed_struct { > struct nested_packed_struct b; > } __attribute__((packed)); > -/* ------ END-EXPECTED-OUTPUT ------ */ > - > -int f(struct { > +struct root_struct { > struct packed_trailing_space _1; > struct non_packed_trailing_space _2; > struct packed_fields _3; > @@ -147,7 +145,11 @@ int f(struct { > struct usb_host_endpoint _10; > struct outer_nonpacked_struct _11; > struct outer_packed_struct _12; > -} *_) > +}; > + > +/* ------ END-EXPECTED-OUTPUT ------ */ > + > +int f(struct root_struct *_) > { > 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..2ca46ad8d66a 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 > @@ -220,9 +220,7 @@ struct outer_mixed_but_unpacked { > struct nested_packed b2; > }; > -/* ------ END-EXPECTED-OUTPUT ------ */ > - > -int f(struct { > +struct root_struct { > struct padded_implicitly _1; > struct padded_explicitly _2; > struct padded_a_lot _3; > @@ -243,7 +241,11 @@ int f(struct { > struct ib_wc _201; > struct acpi_object_method _202; > struct outer_mixed_but_unpacked _203; > -} *_) > +} __attribute__((packed)); > + > +/* ------ END-EXPECTED-OUTPUT ------ */ > + > +int f(struct root_struct *_) > { > return 0; > } > diff --git > a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > index 26fffb02ed10..3e31df7cecc6 100644 > --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > @@ -104,24 +104,24 @@ typedef void (*printf_fn_t)(const char *, ...); > * typedef const fn_output_inner_t fn_ptr_arr2_t[5]; > */ > /* ----- START-EXPECTED-OUTPUT ----- */ > -typedef char * const * (*fn_ptr2_t)(struct { > - int a; > -}, int (*)(int)); > +struct struct_a; > + > +typedef char * const * (*fn_ptr2_t)(struct struct_a, int (*)(int)); > + > +struct struct_c; > + > +struct struct_h; > typedef struct { > int a; > - void (*b)(int, struct { > - int c; > - }, union { > + void (*b)(int, struct struct_c, union { > char d; > int e[5]; > }); > } (*fn_complex_t)(union { > void *f; > char g[16]; > -}, struct { > - int h; > -}); > +}, struct struct_h); > typedef void (* (*signal_t)(int, void (*)(int)))(int); > @@ -272,6 +272,18 @@ struct root_struct { > struct float_struct _15; > }; > +struct struct_a { > + int a; > +}; > + > +struct struct_h { > + int h; > +}; > + > +struct struct_c { > + int c; > +}; > + > /* ------ END-EXPECTED-OUTPUT ------ */ > int f(struct root_struct *s) > -- > 2.34.1
On Tue, Dec 20, 2022 at 9:59 PM James Hilliard <james.hilliard1@gmail.com> wrote: > > Anonymous structs can't be declared inside function parameter > definitions in current c standards, however clang doesn't detect this > condition currently while GCC does. > > Details: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108189 > > Fixes errors like: > progs/btf_dump_test_case_bitfields.c:85:7: error: anonymous struct declared inside parameter list will not be visible outside of this definition or declaration [-Werror] > 85 | int f(struct { > | ^~~~~~ > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > .../bpf/progs/btf_dump_test_case_bitfields.c | 9 ++++-- > .../progs/btf_dump_test_case_namespacing.c | 10 ++++--- > .../bpf/progs/btf_dump_test_case_packing.c | 10 ++++--- > .../bpf/progs/btf_dump_test_case_padding.c | 10 ++++--- > .../bpf/progs/btf_dump_test_case_syntax.c | 30 +++++++++++++------ > 5 files changed, 46 insertions(+), 23 deletions(-) > > 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..c75f6bd06a49 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,16 @@ struct bitfield_flushed { > long b: 16; > }; > > -int f(struct { > +/* ----- START-EXPECTED-OUTPUT ----- */ > +struct root_struct { there is no need to make this struct part of expected output, just keep it next to f? > struct bitfields_only_mixed_types _1; > struct bitfield_mixed_with_others _2; > struct bitfield_flushed _3; > -} *_) > +}; > + > +/* ------ END-EXPECTED-OUTPUT ------ */ > + > +int f(struct root_struct *_) > { > 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..d7cf2a8487c9 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 > @@ -49,9 +49,7 @@ typedef int Y; > > typedef int Z; > > -/*------ END-EXPECTED-OUTPUT ------ */ > - > -int f(struct { > +struct root_struct { > struct S _1; > S _2; > union U _3; > @@ -67,7 +65,11 @@ int f(struct { > X xx; > Y yy; > Z zz; > -} *_) > +}; same, that struct is only to preserve all the referenced types, so keep it hidden from the output > + > +/*------ END-EXPECTED-OUTPUT ------ */ > + > +int f(struct root_struct *_) > { > 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..e039ceb50c43 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 > @@ -132,9 +132,7 @@ struct outer_packed_struct { > struct nested_packed_struct b; > } __attribute__((packed)); > > -/* ------ END-EXPECTED-OUTPUT ------ */ > - > -int f(struct { > +struct root_struct { > struct packed_trailing_space _1; > struct non_packed_trailing_space _2; > struct packed_fields _3; > @@ -147,7 +145,11 @@ int f(struct { > struct usb_host_endpoint _10; > struct outer_nonpacked_struct _11; > struct outer_packed_struct _12; > -} *_) > +}; > + > +/* ------ END-EXPECTED-OUTPUT ------ */ > + > +int f(struct root_struct *_) > { > 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..2ca46ad8d66a 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 > @@ -220,9 +220,7 @@ struct outer_mixed_but_unpacked { > struct nested_packed b2; > }; > > -/* ------ END-EXPECTED-OUTPUT ------ */ > - > -int f(struct { > +struct root_struct { > struct padded_implicitly _1; > struct padded_explicitly _2; > struct padded_a_lot _3; > @@ -243,7 +241,11 @@ int f(struct { > struct ib_wc _201; > struct acpi_object_method _202; > struct outer_mixed_but_unpacked _203; > -} *_) > +} __attribute__((packed)); > + > +/* ------ END-EXPECTED-OUTPUT ------ */ > + > +int f(struct root_struct *_) > { > return 0; > } > diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > index 26fffb02ed10..3e31df7cecc6 100644 > --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > @@ -104,24 +104,24 @@ typedef void (*printf_fn_t)(const char *, ...); > * typedef const fn_output_inner_t fn_ptr_arr2_t[5]; > */ > /* ----- START-EXPECTED-OUTPUT ----- */ > -typedef char * const * (*fn_ptr2_t)(struct { > - int a; > -}, int (*)(int)); > +struct struct_a; > + > +typedef char * const * (*fn_ptr2_t)(struct struct_a, int (*)(int)); > + > +struct struct_c; > + > +struct struct_h; > > typedef struct { > int a; > - void (*b)(int, struct { > - int c; > - }, union { > + void (*b)(int, struct struct_c, union { > char d; > int e[5]; > }); > } (*fn_complex_t)(union { > void *f; > char g[16]; > -}, struct { > - int h; > -}); > +}, struct struct_h); these do test some pieces of libbpf's btf_dump logic, so I'm way more reluctant to remove these. If I understand correctly, this syntax will be eventually supported by GCC, so is there any way to keep these examples as is by requiring C23 mode or something? Or just skipping compiling this one if GCC is used? > > typedef void (* (*signal_t)(int, void (*)(int)))(int); > > @@ -272,6 +272,18 @@ struct root_struct { > struct float_struct _15; > }; > > +struct struct_a { > + int a; > +}; > + > +struct struct_h { > + int h; > +}; > + > +struct struct_c { > + int c; > +}; > + > /* ------ END-EXPECTED-OUTPUT ------ */ > > int f(struct root_struct *s) > -- > 2.34.1 >
On Thu, Dec 22, 2022 at 12:04 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Dec 20, 2022 at 9:59 PM James Hilliard > <james.hilliard1@gmail.com> wrote: > > > > Anonymous structs can't be declared inside function parameter > > definitions in current c standards, however clang doesn't detect this > > condition currently while GCC does. > > > > Details: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108189 > > > > Fixes errors like: > > progs/btf_dump_test_case_bitfields.c:85:7: error: anonymous struct declared inside parameter list will not be visible outside of this definition or declaration [-Werror] > > 85 | int f(struct { > > | ^~~~~~ > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > .../bpf/progs/btf_dump_test_case_bitfields.c | 9 ++++-- > > .../progs/btf_dump_test_case_namespacing.c | 10 ++++--- > > .../bpf/progs/btf_dump_test_case_packing.c | 10 ++++--- > > .../bpf/progs/btf_dump_test_case_padding.c | 10 ++++--- > > .../bpf/progs/btf_dump_test_case_syntax.c | 30 +++++++++++++------ > > 5 files changed, 46 insertions(+), 23 deletions(-) > > > > 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..c75f6bd06a49 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,16 @@ struct bitfield_flushed { > > long b: 16; > > }; > > > > -int f(struct { > > +/* ----- START-EXPECTED-OUTPUT ----- */ > > +struct root_struct { > > there is no need to make this struct part of expected output, just > keep it next to f? Seems to be required as the diff check fails otherwise. > > > > struct bitfields_only_mixed_types _1; > > struct bitfield_mixed_with_others _2; > > struct bitfield_flushed _3; > > -} *_) > > +}; > > + > > +/* ------ END-EXPECTED-OUTPUT ------ */ > > + > > +int f(struct root_struct *_) > > { > > 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..d7cf2a8487c9 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 > > @@ -49,9 +49,7 @@ typedef int Y; > > > > typedef int Z; > > > > -/*------ END-EXPECTED-OUTPUT ------ */ > > - > > -int f(struct { > > +struct root_struct { > > struct S _1; > > S _2; > > union U _3; > > @@ -67,7 +65,11 @@ int f(struct { > > X xx; > > Y yy; > > Z zz; > > -} *_) > > +}; > > same, that struct is only to preserve all the referenced types, so > keep it hidden from the output I wasn't able to find a way to keep it out of the output. The other tests with a root_struct seem to always have it in the output: https://github.com/torvalds/linux/blob/v6.1/tools/testing/selftests/bpf/progs/btf_dump_test_case_multidim.c#L21-L28 https://github.com/torvalds/linux/blob/v6.1/tools/testing/selftests/bpf/progs/btf_dump_test_case_ordering.c#L50-L56 https://github.com/torvalds/linux/blob/v6.1/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c#L222-L237 > > > + > > +/*------ END-EXPECTED-OUTPUT ------ */ > > + > > +int f(struct root_struct *_) > > { > > 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..e039ceb50c43 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 > > @@ -132,9 +132,7 @@ struct outer_packed_struct { > > struct nested_packed_struct b; > > } __attribute__((packed)); > > > > -/* ------ END-EXPECTED-OUTPUT ------ */ > > - > > -int f(struct { > > +struct root_struct { > > struct packed_trailing_space _1; > > struct non_packed_trailing_space _2; > > struct packed_fields _3; > > @@ -147,7 +145,11 @@ int f(struct { > > struct usb_host_endpoint _10; > > struct outer_nonpacked_struct _11; > > struct outer_packed_struct _12; > > -} *_) > > +}; > > + > > +/* ------ END-EXPECTED-OUTPUT ------ */ > > + > > +int f(struct root_struct *_) > > { > > 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..2ca46ad8d66a 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 > > @@ -220,9 +220,7 @@ struct outer_mixed_but_unpacked { > > struct nested_packed b2; > > }; > > > > -/* ------ END-EXPECTED-OUTPUT ------ */ > > - > > -int f(struct { > > +struct root_struct { > > struct padded_implicitly _1; > > struct padded_explicitly _2; > > struct padded_a_lot _3; > > @@ -243,7 +241,11 @@ int f(struct { > > struct ib_wc _201; > > struct acpi_object_method _202; > > struct outer_mixed_but_unpacked _203; > > -} *_) > > +} __attribute__((packed)); > > + > > +/* ------ END-EXPECTED-OUTPUT ------ */ > > + > > +int f(struct root_struct *_) > > { > > return 0; > > } > > diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > > index 26fffb02ed10..3e31df7cecc6 100644 > > --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > > +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > > @@ -104,24 +104,24 @@ typedef void (*printf_fn_t)(const char *, ...); > > * typedef const fn_output_inner_t fn_ptr_arr2_t[5]; > > */ > > /* ----- START-EXPECTED-OUTPUT ----- */ > > -typedef char * const * (*fn_ptr2_t)(struct { > > - int a; > > -}, int (*)(int)); > > +struct struct_a; > > + > > +typedef char * const * (*fn_ptr2_t)(struct struct_a, int (*)(int)); > > + > > +struct struct_c; > > + > > +struct struct_h; > > > > typedef struct { > > int a; > > - void (*b)(int, struct { > > - int c; > > - }, union { > > + void (*b)(int, struct struct_c, union { > > char d; > > int e[5]; > > }); > > } (*fn_complex_t)(union { > > void *f; > > char g[16]; > > -}, struct { > > - int h; > > -}); > > +}, struct struct_h); > > these do test some pieces of libbpf's btf_dump logic, so I'm way more > reluctant to remove these. If I understand correctly, this syntax will > be eventually supported by GCC, so is there any way to keep these > examples as is by requiring C23 mode or something? Or just skipping > compiling this one if GCC is used? I'm not sure, I'm having trouble finding a description in the C23 specification, I presume if it is in there then GCC will eventually support it. Maybe just keep the root_struct changes for now and hold off on this until it's clarified that this is valid C23 code or not? At the moment it appears there's a clang bug here as it shouldn't be valid C17 code(which AFAIU is clang's default). > > > > > typedef void (* (*signal_t)(int, void (*)(int)))(int); > > > > @@ -272,6 +272,18 @@ struct root_struct { > > struct float_struct _15; > > }; > > > > +struct struct_a { > > + int a; > > +}; > > + > > +struct struct_h { > > + int h; > > +}; > > + > > +struct struct_c { > > + int c; > > +}; > > + > > /* ------ END-EXPECTED-OUTPUT ------ */ > > > > int f(struct root_struct *s) > > -- > > 2.34.1 > >
On 12/22/22 11:26 AM, James Hilliard wrote: > On Thu, Dec 22, 2022 at 12:04 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: >> >> On Tue, Dec 20, 2022 at 9:59 PM James Hilliard >> <james.hilliard1@gmail.com> wrote: >>> >>> Anonymous structs can't be declared inside function parameter >>> definitions in current c standards, however clang doesn't detect this >>> condition currently while GCC does. >>> >>> Details: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108189 >>> >>> Fixes errors like: >>> progs/btf_dump_test_case_bitfields.c:85:7: error: anonymous struct declared inside parameter list will not be visible outside of this definition or declaration [-Werror] >>> 85 | int f(struct { >>> | ^~~~~~ >>> >>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>> --- >>> .../bpf/progs/btf_dump_test_case_bitfields.c | 9 ++++-- >>> .../progs/btf_dump_test_case_namespacing.c | 10 ++++--- >>> .../bpf/progs/btf_dump_test_case_packing.c | 10 ++++--- >>> .../bpf/progs/btf_dump_test_case_padding.c | 10 ++++--- >>> .../bpf/progs/btf_dump_test_case_syntax.c | 30 +++++++++++++------ >>> 5 files changed, 46 insertions(+), 23 deletions(-) >>> >>> 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..c75f6bd06a49 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,16 @@ struct bitfield_flushed { >>> long b: 16; >>> }; >>> >>> -int f(struct { >>> +/* ----- START-EXPECTED-OUTPUT ----- */ >>> +struct root_struct { >> >> there is no need to make this struct part of expected output, just >> keep it next to f? > > Seems to be required as the diff check fails otherwise. > >> >> >>> struct bitfields_only_mixed_types _1; >>> struct bitfield_mixed_with_others _2; >>> struct bitfield_flushed _3; >>> -} *_) >>> +}; >>> + >>> +/* ------ END-EXPECTED-OUTPUT ------ */ >>> + >>> +int f(struct root_struct *_) >>> { >>> 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..d7cf2a8487c9 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 >>> @@ -49,9 +49,7 @@ typedef int Y; >>> >>> typedef int Z; >>> >>> -/*------ END-EXPECTED-OUTPUT ------ */ >>> - >>> -int f(struct { >>> +struct root_struct { >>> struct S _1; >>> S _2; >>> union U _3; >>> @@ -67,7 +65,11 @@ int f(struct { >>> X xx; >>> Y yy; >>> Z zz; >>> -} *_) >>> +}; >> >> same, that struct is only to preserve all the referenced types, so >> keep it hidden from the output > > I wasn't able to find a way to keep it out of the output. > > The other tests with a root_struct seem to always have it in the output: > https://github.com/torvalds/linux/blob/v6.1/tools/testing/selftests/bpf/progs/btf_dump_test_case_multidim.c#L21-L28 > https://github.com/torvalds/linux/blob/v6.1/tools/testing/selftests/bpf/progs/btf_dump_test_case_ordering.c#L50-L56 > https://github.com/torvalds/linux/blob/v6.1/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c#L222-L237 > >> >>> + >>> +/*------ END-EXPECTED-OUTPUT ------ */ >>> + >>> +int f(struct root_struct *_) >>> { >>> 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..e039ceb50c43 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 >>> @@ -132,9 +132,7 @@ struct outer_packed_struct { >>> struct nested_packed_struct b; >>> } __attribute__((packed)); >>> >>> -/* ------ END-EXPECTED-OUTPUT ------ */ >>> - >>> -int f(struct { >>> +struct root_struct { >>> struct packed_trailing_space _1; >>> struct non_packed_trailing_space _2; >>> struct packed_fields _3; >>> @@ -147,7 +145,11 @@ int f(struct { >>> struct usb_host_endpoint _10; >>> struct outer_nonpacked_struct _11; >>> struct outer_packed_struct _12; >>> -} *_) >>> +}; >>> + >>> +/* ------ END-EXPECTED-OUTPUT ------ */ >>> + >>> +int f(struct root_struct *_) >>> { >>> 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..2ca46ad8d66a 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 >>> @@ -220,9 +220,7 @@ struct outer_mixed_but_unpacked { >>> struct nested_packed b2; >>> }; >>> >>> -/* ------ END-EXPECTED-OUTPUT ------ */ >>> - >>> -int f(struct { >>> +struct root_struct { >>> struct padded_implicitly _1; >>> struct padded_explicitly _2; >>> struct padded_a_lot _3; >>> @@ -243,7 +241,11 @@ int f(struct { >>> struct ib_wc _201; >>> struct acpi_object_method _202; >>> struct outer_mixed_but_unpacked _203; >>> -} *_) >>> +} __attribute__((packed)); >>> + >>> +/* ------ END-EXPECTED-OUTPUT ------ */ >>> + >>> +int f(struct root_struct *_) >>> { >>> return 0; >>> } >>> diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c >>> index 26fffb02ed10..3e31df7cecc6 100644 >>> --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c >>> +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c >>> @@ -104,24 +104,24 @@ typedef void (*printf_fn_t)(const char *, ...); >>> * typedef const fn_output_inner_t fn_ptr_arr2_t[5]; >>> */ >>> /* ----- START-EXPECTED-OUTPUT ----- */ >>> -typedef char * const * (*fn_ptr2_t)(struct { >>> - int a; >>> -}, int (*)(int)); >>> +struct struct_a; >>> + >>> +typedef char * const * (*fn_ptr2_t)(struct struct_a, int (*)(int)); >>> + >>> +struct struct_c; >>> + >>> +struct struct_h; >>> >>> typedef struct { >>> int a; >>> - void (*b)(int, struct { >>> - int c; >>> - }, union { >>> + void (*b)(int, struct struct_c, union { >>> char d; >>> int e[5]; >>> }); >>> } (*fn_complex_t)(union { >>> void *f; >>> char g[16]; >>> -}, struct { >>> - int h; >>> -}); >>> +}, struct struct_h); >> >> these do test some pieces of libbpf's btf_dump logic, so I'm way more >> reluctant to remove these. If I understand correctly, this syntax will >> be eventually supported by GCC, so is there any way to keep these >> examples as is by requiring C23 mode or something? Or just skipping >> compiling this one if GCC is used? > > I'm not sure, I'm having trouble finding a description in the C23 specification, > I presume if it is in there then GCC will eventually support it. > > Maybe just keep the root_struct changes for now and hold off on this until > it's clarified that this is valid C23 code or not? > > At the moment it appears there's a clang bug here as it shouldn't be valid > C17 code(which AFAIU is clang's default). Looks like clang supports anonymous struct parameter all the way back to c89. I tried clang12 and latest clang16. $ cat t.c typedef char * const * (*fn_ptr2_t)(struct { int a; }, int (*)(int)); fn_ptr2_t h; $ clang -S -emit-llvm -g -std=c89 t.c $ clang -S -emit-llvm -g -std=c11 t.c $ clang -S -emit-llvm -g -std=c17 t.c ]$ clang -S -emit-llvm -g t.c $ gcc -v -S t.c ... GNU C17 (GCC) version 8.5.0 20210514 (Red Hat 8.5.0-17) (x86_64-redhat-linux) ... t.c:1:37: warning: anonymous struct declared inside parameter list will not be visible outside of this definition or declaration typedef char * const * (*fn_ptr2_t)(struct { ^~~~~~ ... I agree that we should understand better about standard requirement before making the change. > >> >>> >>> typedef void (* (*signal_t)(int, void (*)(int)))(int); >>> >>> @@ -272,6 +272,18 @@ struct root_struct { >>> struct float_struct _15; >>> }; >>> >>> +struct struct_a { >>> + int a; >>> +}; >>> + >>> +struct struct_h { >>> + int h; >>> +}; >>> + >>> +struct struct_c { >>> + int c; >>> +}; >>> + >>> /* ------ END-EXPECTED-OUTPUT ------ */ >>> >>> int f(struct root_struct *s) >>> -- >>> 2.34.1 >>>
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..c75f6bd06a49 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,16 @@ struct bitfield_flushed { long b: 16; }; -int f(struct { +/* ----- START-EXPECTED-OUTPUT ----- */ +struct root_struct { struct bitfields_only_mixed_types _1; struct bitfield_mixed_with_others _2; struct bitfield_flushed _3; -} *_) +}; + +/* ------ END-EXPECTED-OUTPUT ------ */ + +int f(struct root_struct *_) { 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..d7cf2a8487c9 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 @@ -49,9 +49,7 @@ typedef int Y; typedef int Z; -/*------ END-EXPECTED-OUTPUT ------ */ - -int f(struct { +struct root_struct { struct S _1; S _2; union U _3; @@ -67,7 +65,11 @@ int f(struct { X xx; Y yy; Z zz; -} *_) +}; + +/*------ END-EXPECTED-OUTPUT ------ */ + +int f(struct root_struct *_) { 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..e039ceb50c43 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 @@ -132,9 +132,7 @@ struct outer_packed_struct { struct nested_packed_struct b; } __attribute__((packed)); -/* ------ END-EXPECTED-OUTPUT ------ */ - -int f(struct { +struct root_struct { struct packed_trailing_space _1; struct non_packed_trailing_space _2; struct packed_fields _3; @@ -147,7 +145,11 @@ int f(struct { struct usb_host_endpoint _10; struct outer_nonpacked_struct _11; struct outer_packed_struct _12; -} *_) +}; + +/* ------ END-EXPECTED-OUTPUT ------ */ + +int f(struct root_struct *_) { 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..2ca46ad8d66a 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 @@ -220,9 +220,7 @@ struct outer_mixed_but_unpacked { struct nested_packed b2; }; -/* ------ END-EXPECTED-OUTPUT ------ */ - -int f(struct { +struct root_struct { struct padded_implicitly _1; struct padded_explicitly _2; struct padded_a_lot _3; @@ -243,7 +241,11 @@ int f(struct { struct ib_wc _201; struct acpi_object_method _202; struct outer_mixed_but_unpacked _203; -} *_) +} __attribute__((packed)); + +/* ------ END-EXPECTED-OUTPUT ------ */ + +int f(struct root_struct *_) { return 0; } diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c index 26fffb02ed10..3e31df7cecc6 100644 --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c @@ -104,24 +104,24 @@ typedef void (*printf_fn_t)(const char *, ...); * typedef const fn_output_inner_t fn_ptr_arr2_t[5]; */ /* ----- START-EXPECTED-OUTPUT ----- */ -typedef char * const * (*fn_ptr2_t)(struct { - int a; -}, int (*)(int)); +struct struct_a; + +typedef char * const * (*fn_ptr2_t)(struct struct_a, int (*)(int)); + +struct struct_c; + +struct struct_h; typedef struct { int a; - void (*b)(int, struct { - int c; - }, union { + void (*b)(int, struct struct_c, union { char d; int e[5]; }); } (*fn_complex_t)(union { void *f; char g[16]; -}, struct { - int h; -}); +}, struct struct_h); typedef void (* (*signal_t)(int, void (*)(int)))(int); @@ -272,6 +272,18 @@ struct root_struct { struct float_struct _15; }; +struct struct_a { + int a; +}; + +struct struct_h { + int h; +}; + +struct struct_c { + int c; +}; + /* ------ END-EXPECTED-OUTPUT ------ */ int f(struct root_struct *s)
Anonymous structs can't be declared inside function parameter definitions in current c standards, however clang doesn't detect this condition currently while GCC does. Details: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108189 Fixes errors like: progs/btf_dump_test_case_bitfields.c:85:7: error: anonymous struct declared inside parameter list will not be visible outside of this definition or declaration [-Werror] 85 | int f(struct { | ^~~~~~ Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- .../bpf/progs/btf_dump_test_case_bitfields.c | 9 ++++-- .../progs/btf_dump_test_case_namespacing.c | 10 ++++--- .../bpf/progs/btf_dump_test_case_packing.c | 10 ++++--- .../bpf/progs/btf_dump_test_case_padding.c | 10 ++++--- .../bpf/progs/btf_dump_test_case_syntax.c | 30 +++++++++++++------ 5 files changed, 46 insertions(+), 23 deletions(-)