On Mon, 2022-11-14 at 11:56 -0800, Andrii Nakryiko wrote:
> On Fri, Nov 11, 2022 at 1:30 PM Eduard Zingerman <eddyz87@xxxxxxxxx> 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@xxxxxxxxx> 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
> > > >
> >
