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

[Eduard Zingerman <eddyz87@gmail.com>]

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:

  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?

> 
> > @@ -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.

> 
> >         ret = btf_dump_dump_type_data(d, NULL, t, id, data, 0, 0);
> > 
> >         d->typed_dump = NULL;
> > --
> > 2.34.1
> >