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

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2022-11-14 at 11:56 -0800, Andrii Nakryiko wrote:
> On Fri, Nov 11, 2022 at 1:30 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Fri, 2022-11-11 at 10:58 -0800, Andrii Nakryiko wrote:
> > > On Thu, Nov 10, 2022 at 6:43 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > > 
> > > 
> > > [...]
> > > 
> > > >  static int btf_dump_push_decl_stack_id(struct btf_dump *d, __u32 id)
> > > > @@ -1438,9 +1593,12 @@ static void btf_dump_emit_type_chain(struct btf_dump *d,
> > > >                 }
> > > >                 case BTF_KIND_FUNC_PROTO: {
> > > >                         const struct btf_param *p = btf_params(t);
> > > > +                       struct decl_tag_array *decl_tags = NULL;
> > > >                         __u16 vlen = btf_vlen(t);
> > > >                         int i;
> > > > 
> > > > +                       hashmap__find(d->decl_tags, id, &decl_tags);
> > > > +
> > > >                         /*
> > > >                          * GCC emits extra volatile qualifier for
> > > >                          * __attribute__((noreturn)) function pointers. Clang
> > > 
> > > should there be btf_dump_emit_decl_tags(d, decl_tags, -1) somewhere
> > > here to emit tags of FUNC_PROTO itself?
> > 
> > Actually, I have not found a way to attach decl tag to a FUNC_PROTO itself:
> 
> I'll need to check with Yonghong, but I think what happens right now
> with decl_tag being attached to FUNC instead of its underlying
> FUNC_PROTO might be a bug (or maybe it's by design, but certainly is
> quite confusing as FUNC itself doesn't have arguments, so
> component_idx != -1 is a bit weird).
> 
> But regardless if Clang allows you to express it in C code today or
> not, if we support decl_tags on func proto args, for completeness
> let's support it also on func_proto itself (comp_idx == -1). You can
> build BTF manually for test, just like you do it for func_proto args,
> right?

I can construct the BTF manually, but I need a place in C where
__decl_tag would be printed for such proto and currently there is no
such place.

As Yonghong suggests in a sibling comment there are currently no
use-cases for decl tags on functions, function protos or function
proto parameters. I suggest to drop these places from the current
patch and get back to it when the need arises.

> > 
> >   typedef void (*fn)(void) __decl_tag("..."); // here tag is attached to typedef
> >   struct foo {
> >     void (*fn)(void) __decl_tag("..."); // here tag is attached to a foo.fn field
> >   }
> >   void foo(void (*fn)(void) __decl_tag("...")); // here tag is attached to FUNC foo
> >                                                 // parameter but should probably
> >                                                 // be attached to
> >                                                 // FUNC_PROTO parameter instead.
> > 
> > Also, I think that Yonghong had reservations about decl tags attached to
> > FUNC_PROTO parameters.
> > Yonghong, could you please comment?
> 
> yep, curious to hear as well
> 
> 
> > 
> > > 
> > > > @@ -1481,6 +1639,7 @@ static void btf_dump_emit_type_chain(struct btf_dump *d,
> > > > 
> > > >                                 name = btf_name_of(d, p->name_off);
> > > >                                 btf_dump_emit_type_decl(d, p->type, name, lvl);
> > > > +                               btf_dump_emit_decl_tags(d, decl_tags, i);
> > > >                         }
> > > > 
> > > >                         btf_dump_printf(d, ")");
> > > > @@ -1896,6 +2055,7 @@ static int btf_dump_var_data(struct btf_dump *d,
> > > >                              const void *data)
> > > >  {
> > > >         enum btf_func_linkage linkage = btf_var(v)->linkage;
> > > > +       struct decl_tag_array *decl_tags = NULL;
> > > >         const struct btf_type *t;
> > > >         const char *l;
> > > >         __u32 type_id;
> > > > @@ -1920,7 +2080,10 @@ static int btf_dump_var_data(struct btf_dump *d,
> > > >         type_id = v->type;
> > > >         t = btf__type_by_id(d->btf, type_id);
> > > >         btf_dump_emit_type_cast(d, type_id, false);
> > > > -       btf_dump_printf(d, " %s = ", btf_name_of(d, v->name_off));
> > > > +       btf_dump_printf(d, " %s", btf_name_of(d, v->name_off));
> > > > +       hashmap__find(d->decl_tags, id, &decl_tags);
> > > > +       btf_dump_emit_decl_tags(d, decl_tags, -1);
> > > > +       btf_dump_printf(d, " = ");
> > > >         return btf_dump_dump_type_data(d, NULL, t, type_id, data, 0, 0);
> > > >  }
> > > > 
> > > > @@ -2421,6 +2584,8 @@ int btf_dump__dump_type_data(struct btf_dump *d, __u32 id,
> > > >         d->typed_dump->skip_names = OPTS_GET(opts, skip_names, false);
> > > >         d->typed_dump->emit_zeroes = OPTS_GET(opts, emit_zeroes, false);
> > > > 
> > > > +       btf_dump_assign_decl_tags(d);
> > > > +
> > > 
> > > I'm actually not sure we want those tags on binary data dump.
> > > Generally data dump is not type definition dump, so this seems
> > > unnecessary, it will just distract from data itself. Let's drop it for
> > > now? If there would be a need we can add it easily later.
> > 
> > Well, this is the only place where VARs are processed, removing this code
> > would make the second patch in a series useless.
> > But I like my second patch in a series :) should I just drop it?
> > I can extract it as a separate series and simplify some of the existing
> > data dump tests.
> 
> yep, data dump tests can be completely orthogonal, send them
> separately if you are attached to that code ;)
> 
> but for decl_tags on dump_type_data() I'd rather be conservative for
> now, unless in practice those decl_tags will turn out to be needed
> 
> 
> > 
> > > 
> > > >         ret = btf_dump_dump_type_data(d, NULL, t, id, data, 0, 0);
> > > > 
> > > >         d->typed_dump = NULL;
> > > > --
> > > > 2.34.1
> > > > 
> >