Anonymous struct types in parameter lists in BPF selftests

Anonymous struct types in parameter lists in BPF selftests

[Jose E. Marchesi]

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.

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 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?

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;
 }

Re: Anonymous struct types in parameter lists in BPF selftests

[Eduard Zingerman]

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.

Re: Anonymous struct types in parameter lists in BPF selftests

[Andrii Nakryiko]

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

[...]

Re: Anonymous struct types in parameter lists in BPF selftests

[Jose E. Marchesi]

> 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
>
> [...]