On Thu, Mar 18, 2021 at 5:31 AM Jean-Philippe Brucker
<jean-philippe@xxxxxxxxxx> wrote:
>
> The vmlinux.h generated from BTF is invalid when building
> drivers/phy/ti/phy-gmii-sel.c with clang:
>
> vmlinux.h:61702:27: error: array type has incomplete element type ‘struct reg_field’
> 61702 | const struct reg_field (*regfields)[3];
> | ^~~~~~~~~
>
> bpftool generates a forward declaration for this struct regfield, which
> compilers aren't happy about. Here's a simplified reproducer:
>
> struct inner {
> int val;
> };
> struct outer {
> struct inner (*ptr_to_array)[2];
> };
>
> static struct inner a[2];
> struct outer b = {
> .ptr_to_array = &a,
> };
>
> After build with clang -> bpftool btf dump c -> clang/gcc:
> ./def-clang.h:11:23: error: array has incomplete element type 'struct inner'
> struct inner (*ptr_to_array)[2];
>
> Member ptr_to_array of struct outer is a pointer to an array of struct
> inner. In the DWARF generated by clang, struct outer appears before
> struct inner, so when converting BTF of struct outer into C, bpftool
> issues a forward declaration of struct inner. With GCC the DWARF info is
> reversed so struct inner gets fully defined.
>
> That forward declaration is not sufficient when compilers handle an
> array of the struct, even when it's only used through a pointer. Note
> that we can trigger the same issue with an intermediate typedef:
>
> struct inner {
> int val;
> };
> typedef struct inner inner2_t[2];
> struct outer {
> inner2_t *ptr_to_array;
> };
>
> static inner2_t a;
> struct outer b = {
> .ptr_to_array = &a,
> };
>
> Becomes:
>
> struct inner;
> typedef struct inner inner2_t[2];
>
> And causes:
>
> ./def-clang.h:10:30: error: array has incomplete element type 'struct inner'
> typedef struct inner inner2_t[2];
>
> To fix this, clear through_ptr whenever we encounter an intermediate
> array, to make the inner struct part of a strong link and force full
> declaration.
>
Yeah, makes total sense. I missed that array forces a strong link
between types. The fix looks good, but can you please add those two
cases to selftests? There is progs/btf_dump_test_case_syntax.c that
probably can be extended. Please think about a way to specify types
such that the order of BTF types doesn't matter and the issue has to
be handled always.
> Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> ---
> tools/lib/bpf/btf_dump.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 2f9d685bd522..0911aea4cdbe 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -462,7 +462,7 @@ static int btf_dump_order_type(struct btf_dump *d, __u32 id, bool through_ptr)
> return err;
>
> case BTF_KIND_ARRAY:
> - return btf_dump_order_type(d, btf_array(t)->type, through_ptr);
> + return btf_dump_order_type(d, btf_array(t)->type, false);
>
> case BTF_KIND_STRUCT:
> case BTF_KIND_UNION: {
> --
> 2.30.2
>
