> On Thu, Jan 25, 2024 at 3:31 AM Jose E. Marchesi
> <jose.marchesi@xxxxxxxxxx> 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
>
> [...]
