On Mon, 2022-11-07 at 15:09 -0800, Andrii Nakryiko wrote:
> On Mon, Nov 7, 2022 at 7:35 AM Eduard Zingerman <eddyz87@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > > 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')
> >
> > > [...]
> > >
>
> [...]
