Re: [PATCH bpf-next 1/2] libbpf: __attribute__((btf_decl_tag("..."))) for btf dump in C format

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2022-11-07 at 15:09 -0800, Andrii Nakryiko wrote:
> On Mon, Nov 7, 2022 at 7:35 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Fri, 2022-11-04 at 13:54 -0700, Andrii Nakryiko wrote:
> > > On Thu, Nov 3, 2022 at 6:45 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > > > 
> > > > Clang's `__attribute__((btf_decl_tag("...")))` is represented in BTF
> > > > as a record of kind BTF_KIND_DECL_TAG with `type` field pointing to
> > > > the type annotated with this attribute. This commit adds
> > > > reconstitution of such attributes for BTF dump in C format.
> > > > 
> > > > BTF doc says that BTF_KIND_DECL_TAGs should follow a target type but
> > > > this is not enforced and tests don't honor this restriction.
> > > > This commit uses hashmap to map types to the list of decl tags.
> > > > The hashmap is filled by `btf_dump_assign_decl_tags` function called
> > > > from `btf_dump__new`.
> > > > 
> > > > It is assumed that total number of types annotated with decl tags is
> > > > relatively small, thus some space is saved by using hashmap instead of
> > > > adding a new field to `struct btf_dump_type_aux_state`.
> > > > 
> > > > It is assumed that list of decl tags associated with a single type is
> > > > small. Thus the list is represented by an array which grows linearly.
> > > > 
> > > > To accommodate older Clang versions decl tags are dumped using the
> > > > following macro:
> > > > 
> > > >  #if __has_attribute(btf_decl_tag)
> > > >  #  define __btf_decl_tag(x) __attribute__((btf_decl_tag(x)))
> > > >  #else
> > > >  #  define __btf_decl_tag(x)
> > > >  #endif
> > > > 
> > > > The macro definition is emitted upon first call to `btf_dump__dump_type`.
> > > > 
> > > > Clang allows to attach btf_decl_tag attributes to the following kinds
> > > > of items:
> > > > - struct/union         supported
> > > > - struct/union field   supported
> > > > - typedef              supported
> > > > - function             not applicable
> > > > - function parameter   not applicable
> > > > - variable             not applicable
> > > > 
> > > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > > > ---
> > > >  tools/lib/bpf/btf_dump.c | 163 ++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 160 insertions(+), 3 deletions(-)
> > > > 
> > > 
> > > Functions and their args can also have tags. This works:
> > > 
> > > diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c
> > > b/tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c
> > > index 7a5af8b86065..75fcabe700cd 100644
> > > --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c
> > > +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c
> > > @@ -54,7 +54,7 @@ struct root_struct {
> > > 
> > >  /* ------ END-EXPECTED-OUTPUT ------ */
> > > 
> > > -int f(struct root_struct *s)
> > > +int f(struct root_struct *s __btf_decl_tag("func_arg_tag"))
> > > __btf_decl_tag("func_tag")
> > >  {
> > >         return 0;
> > >  }
> > > 
> > > And I see correct BTF:
> > > 
> > > [26] FUNC 'f' type_id=25 linkage=global
> > > [27] DECL_TAG 'func_arg_tag' type_id=26 component_idx=0
> > > [28] DECL_TAG 'func_tag' type_id=26 component_idx=-1
> > > 
> > > So let's add support and test for that case as well. btf_dump
> > > shouldn't assume vmlinux.h-only case.
> > > 
> > > Also, please check if DATASEC and VARs can have decl_tags associated with them.
> > 
> > I see that right now decl tags are saved for:
> > - BTF_KIND_VAR
> > - BTF_KIND_FUNC
> > - BTF_KIND_FUNC arguments
> > 
> > Decl tags are lost but legal for:
> > - BTF_KIND_FUNC_PROTO arguments
> 
> ah, that's unfortunate and even DECL_TAGS example I showed above seems
> like a bug. FUNC itself doesn't have args, I implicitly assumed that
> all DECL_TAG will be actually associated with underlying FUNC_PROTO.
> 
> Yonghong, is this by design or a bug?
> 
> > 
> > I have not found a way to attach decl tag to DATASEC.
> > 
> > For BTF_KIND_FUNC_PROTO  arguments it would  be great to  update clang
> > first. Then  it would be  possible to keep all  decl tags checks  as a
> > single  `btf_dump_test_case`.  On  the   other  hand  this  will  make
> > testsuite dependent on the latest clang version, which is not great. I
> > can add a test with hand-crafted BTF instead. Which way is preferable?
> 
> let's figure out if current state is accidental or by design.
> 
> From practical standpoint, I'd still implement the code for FUNC_PROTO
> and its args, but I wouldn't go all the way to hand-craft BTF
> programmatically. As you said, btf_dump tests are way more ergonomic
> because we rely on compiler to do the heavy lifting.
> 
> As for the dependency on latest clang for some tests, I think that's
> totally fine and unavoidable. Worst case some subtests will fail on
> old kernels, they can be denylisted on systems with old compiler. All
> that won't break the build (which is much worse and inconvenient).
> 
> > 
> > BTF_KIND_FUNC is ignored by `btf_dump__dump_type_data`
> > (via `btf_dump_unsupported_data`).
> > 
> > BTF_KIND_VAR is dumped but current  testing infrastructure is not very
> > convenient, it only checks for  some variables defined in vmlinux BTF.
> > I can write a  test that accepts a custom built BTF  but this is still
> > inferior   to  what   `test_btf_dump_case`  provides.   I've  extended
> > `test_btf_dump_case` to print DATASEC  with subordinate vars alongside
> > the type definitions instead.
> > 
> 
> dumping DATASEC/VAR and FUNC is something that seems useful in
> general, but we should treat it as a separate problem. Seeing DATASEC
> variables and FUNCs in a familiar C syntax would be nice, but it
> probably should be guarded behind a bpftool option or something.
> 
> So in summary, let's figure out the situation with FUNC and FUNC_PROTO
> first, and let's not due too laborious selftests yet

Actually, to test the decl tag attachment for VAR I already implemented
a change to the `test_btf_dump_case`. It can be separated as a different
kind of tests but I don't see a point as it would be very similar to
`test_btf_dump_case`.

And manually creating BTF to attach decl tag to function proto parameter
highlighted an issue that `btf_dump_assign_decl_tags` is only called once.
So, the incremental scenario is not supported.

I can post v2 with the change to `test_btf_dump_case`, support for
decl tags on VARs and a fix for incremental dump behavior.

> 
> > ------
> > 
> > $ cat test.c
> > #define __btf_decl_tag(x) __attribute__((btf_decl_tag(x)))
> > 
> > int var __btf_decl_tag("var_tag");
> > 
> > struct root {
> >   int a;
> >   int (*b)(int x __btf_decl_tag("arg_tag_proto")) __btf_decl_tag("field_tag");
> > };
> > 
> > int foo(struct root *x __btf_decl_tag("arg_tag_fn")) __btf_decl_tag("func_tag_fn") {
> >   return 0;
> > }
> > $ clang -g -O2 -mcpu=v3 -target bpf -c test.c -o test.o
> > $ bpftool btf dump file test.o
> > [1] PTR '(anon)' type_id=2
> > [2] STRUCT 'root' size=16 vlen=2
> >         'a' type_id=3 bits_offset=0
> >         'b' type_id=4 bits_offset=64
> > [3] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > [4] PTR '(anon)' type_id=5
> > [5] FUNC_PROTO '(anon)' ret_type_id=3 vlen=1
> >         '(anon)' type_id=3
> > [6] DECL_TAG 'field_tag' type_id=2 component_idx=1
> > [7] FUNC_PROTO '(anon)' ret_type_id=3 vlen=1
> >         'x' type_id=1
> > [8] FUNC 'foo' type_id=7 linkage=global
> > [9] DECL_TAG 'arg_tag_fn' type_id=8 component_idx=0
> > [10] DECL_TAG 'func_tag_fn' type_id=8 component_idx=-1
> > [11] VAR 'var' type_id=3, linkage=global
> > [12] DECL_TAG 'var_tag' type_id=11 component_idx=-1
> > [13] DATASEC '.bss' size=0 vlen=1
> >         type_id=11 offset=0 size=4 (VAR 'var')
> > 
> > > [...]
> > > 
> 
> [...]