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