diff mbox series

[bpf-next] selftests/bpf: move struct definitions out of function params

Message ID 20221221055856.2786043-1-james.hilliard1@gmail.com
State New
Headers
Series [bpf-next] selftests/bpf: move struct definitions out of function params |

Commit Message

James Hilliard Dec. 21, 2022, 5:58 a.m. UTC 
  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(-)

Comments

Stanislav Fomichev Dec. 21, 2022, 7:07 p.m. UTC | #1 
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
Andrii Nakryiko Dec. 22, 2022, 7:03 p.m. UTC | #2 
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
>
James Hilliard Dec. 22, 2022, 7:26 p.m. UTC | #3 
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
> >
Yonghong Song Dec. 28, 2022, 6:42 p.m. UTC | #4 
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 mbox series

Patch

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)