Re: Anonymous struct types in parameter lists in BPF selftests

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

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