On Tue, Nov 15, 2022 at 5:51 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
after func prototype definition:
$ cat t.c
#include <stdio.h>
typedef int (* ff)(void *arg) __attribute__((nonnull(1)));
static
int blah(void *x) { return (int)(long)x; }
int main() {
int (*f1)(void *arg) __attribute__((nonnull(1))) = blah;
ff f2 = blah;
blah(NULL);
f1(NULL);
f2(NULL);
printf("%lx %lx\n", (long)f1, (long)f2);
return 0;
}
$ cc -g t.c -Wnonnull && ./a.out
t.c: In function ‘main’:
t.c:13:9: warning: argument 1 null where non-null expected [-Wnonnull]
13 | f1(NULL);
| ^~
t.c:14:9: warning: argument 1 null where non-null expected [-Wnonnull]
14 | f2(NULL);
| ^~
401126 401126
Note that blah(NULL) doesn't generate a warning, which means nonnull
attributes are applied only to func_proto.
>
> 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.
decl_tags for functions and function protos are natural extensions of
decl_tags for fields/structs/variables, so let's do the proper support
for all conceivable use cases instead of doing this in a few months
again. There is not ambiguity here.
And btw, we do have decl_tags for FUNCs right now, and that seems
wrong, because FUNC itself doesn't have arguments, it only points to
FUNC_PROTO. So it seems like decl_tags should be moved to FUNC_PROTO
instead anyways?
>
> > >
> > > 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
> > > > >
> > >
>
