On Tue, Oct 25, 2022 at 3:28 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> At C level BTF_DECL_TAGs are represented as __attribute__
> declarations, e.g.:
>
> struct foo {
> ...;
> } __attribute__((btf_decl_tag("bar")));
>
> This commit covers only decl tags attached to structs and unions.
>
> BTF doc says that BTF_DECL_TAGs should follow a target type but this
> is not enforced and tests don't honor this restriction.
> This commit uses hash table to map types to the list of decl tags.
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> tools/lib/bpf/btf_dump.c | 143 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 142 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index bf0cc0e986dd..9bfe2a4ae277 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -75,6 +75,15 @@ struct btf_dump_data {
> bool is_array_char;
> };
>
> +/*
> + * An array of ids of BTF_DECL_TAG objects associated with a specific type.
> + */
> +struct decl_tag_array {
> + __u16 cnt;
> + __u16 cap;
> + __u32 tag_ids[0];
> +};
> +
> struct btf_dump {
> const struct btf *btf;
> btf_dump_printf_fn_t printf_fn;
> @@ -111,6 +120,11 @@ struct btf_dump {
> * name occurrences
> */
> struct hashmap *ident_names;
> + /*
> + * maps type id to decl_tag_array, assume that relatively small
> + * fraction of types has btf_decl_tag's attached
> + */
> + struct hashmap *decl_tags;
> /*
> * data for typed display; allocated if needed.
> */
> @@ -127,6 +141,26 @@ static bool str_equal_fn(const void *a, const void *b, void *ctx)
> return strcmp(a, b) == 0;
> }
>
> +static size_t int_hash_fn(const void *key, void *ctx)
> +{
> + int i;
> + size_t h = 0;
> + char *bytes = (char *)key;
> +
> + for (i = 0; i < 4; ++i)
> + h = h * 31 + bytes[i];
> +
> + return h;
> +}
no need, you can just do what btf_dedup_identity_hash_fn() is doing
and pass int/long/size_t as is, hashmap implementation does additional
multiplicative hashing on top to find a bucket
> +
> +static bool int_equal_fn(const void *a, const void *b, void *ctx)
> +{
> + int *ia = (int *)a;
> + int *ib = (int *)b;
> +
> + return *ia == *ib;
> +}
see btf_dedup_equal_fn(), no need for casting, just return a == b;
> +
> static const char *btf_name_of(const struct btf_dump *d, __u32 name_off)
> {
> return btf__name_by_offset(d->btf, name_off);
> @@ -143,6 +177,7 @@ static void btf_dump_printf(const struct btf_dump *d, const char *fmt, ...)
>
> static int btf_dump_mark_referenced(struct btf_dump *d);
> static int btf_dump_resize(struct btf_dump *d);
> +static int btf_dump_assign_decl_tags(struct btf_dump *d);
>
> struct btf_dump *btf_dump__new(const struct btf *btf,
> btf_dump_printf_fn_t printf_fn,
> @@ -179,11 +214,24 @@ struct btf_dump *btf_dump__new(const struct btf *btf,
> d->ident_names = NULL;
> goto err;
> }
> + d->decl_tags = hashmap__new(int_hash_fn, int_equal_fn, NULL);
> + if (IS_ERR(d->decl_tags)) {
> + err = PTR_ERR(d->decl_tags);
> + d->decl_tags = NULL;
> + goto err;
> + }
>
> err = btf_dump_resize(d);
> if (err)
> goto err;
>
> + err = btf_dump_assign_decl_tags(d);
> + if (err)
> + goto err;
> +
> + if (err)
> + goto err;
> +
I like the bullet-proof error checking, but checking just once should
be enough ;)
> return d;
> err:
> btf_dump__free(d);
> @@ -232,7 +280,8 @@ static void btf_dump_free_names(struct hashmap *map)
>
> void btf_dump__free(struct btf_dump *d)
> {
> - int i;
> + int i, bkt;
> + struct hashmap_entry *cur;
>
> if (IS_ERR_OR_NULL(d))
> return;
> @@ -250,6 +299,9 @@ void btf_dump__free(struct btf_dump *d)
> free(d->decl_stack);
> btf_dump_free_names(d->type_names);
> btf_dump_free_names(d->ident_names);
> + hashmap__for_each_entry(d->decl_tags, cur, bkt)
> + free(cur->value);
> + hashmap__free(d->decl_tags);
>
> free(d);
> }
> @@ -373,6 +425,77 @@ static int btf_dump_mark_referenced(struct btf_dump *d)
> return 0;
> }
>
> +static struct decl_tag_array *btf_dump_find_decl_tags(struct btf_dump *d, __u32 id)
do we really need this wrapper?
> +{
> + struct decl_tag_array *decl_tags = NULL;
> +
> + hashmap__find(d->decl_tags, &id, (void **)&decl_tags);
this &id also made me realize that this is all broken, you are
remembering random pointers in hashmap (they point onto stack, which
gets reused once this function returns; but hashmap remember it, so on
next lookup or update we are going to be reading random values in
int_equal_fn?)
you should be passing (void *)(long)id instead (and better yet let's
refactor hashmap API as I suggested in previous patch)
either I'm missing something, or this works by accident, which
suggests that tests could be improved maybe?..
> +
> + return decl_tags;
> +}
> +
> +static struct decl_tag_array *realloc_decl_tags(struct decl_tag_array *tags, __u16 new_cap)
> +{
> + size_t new_size = sizeof(struct decl_tag_array) + new_cap * sizeof(__u32);
> + struct decl_tag_array *new_tags = (tags
> + ? realloc(tags, new_size)
> + : calloc(1, new_size));
realloc allocates if passed NULL, so no need for calloc, assuming
proper initialization
but let's use libbpf_reallocarray(), we'll waste few bytes on size_t,
but given we expect few tags, it's not a big deal
> +
> + if (!new_tags)
> + return NULL;
> +
> + new_tags->cap = new_cap;
> +
> + return new_tags;
> +}
> +
> +/*
> + * Scans all BTF objects looking for BTF_KIND_DECL_TAG entries.
> + * The id's of the entries are stored in the `btf_dump.decl_tags` table,
> + * grouped by a target type.
> + */
> +static int btf_dump_assign_decl_tags(struct btf_dump *d)
> +{
> + int err;
> + __u32 id;
> + __u32 n = btf__type_cnt(d->btf);
> + __u32 new_capacity;
> + const struct btf_type *t;
> + struct decl_tag_array *decl_tags;
few nits: generally, for new code try to do reverse Christmas try
style, where widest line is at the top, shortest at the bottom
but also here you can have id, new_capacity, and n on same line
and s/new_capacity/new_cap/
> +
> + for (id = 0; id < n; id++) {
0 is VOID, we never really need to process it, just start with id = 1
> + t = btf__type_by_id(d->btf, id);
> +
> + if (btf_kind(t) != BTF_KIND_DECL_TAG)
> + continue;
if (!btf_is_decl_tag(t))
continue;
> +
> + decl_tags = btf_dump_find_decl_tags(d, t->type);
> + if (!decl_tags) {
> + decl_tags = realloc_decl_tags(NULL, 1);
> + if (!decl_tags)
> + return -ENOMEM;
> + err = hashmap__insert(d->decl_tags, &t->type, decl_tags,
> + HASHMAP_SET, NULL, NULL);
> + if (err)
> + return err;
> + } else if (decl_tags->cnt == decl_tags->cap) {
> + new_capacity = decl_tags->cap * 2;
> + if (new_capacity > 0xffff)
> + return -ERANGE;
> + decl_tags = realloc_decl_tags(decl_tags, new_capacity);
> + if (!decl_tags)
> + return -ENOMEM;
> + decl_tags->cap = new_capacity;
> + err = hashmap__update(d->decl_tags, &t->type, decl_tags, NULL, NULL);
> + if (err)
> + return err;
> + }
really, let's just use libbpf_reallocarray? I was going to suggest
libbpf_ensure_mem, but it allocates at least 16 elements, which seems
like an overkill. But also given we don't expect a lot of tags per
type, realloc()'ing with + 1 (no * 2 strategy) seems reasonable.
Modern allocators either way use differently sized buckets, so when
realloc size increment is small, allocator basically will do nothing.
> + decl_tags->tag_ids[decl_tags->cnt++] = id;
> + }
> +
> + return 0;
> +}
> +
> static int btf_dump_add_emit_queue_id(struct btf_dump *d, __u32 id)
> {
> __u32 *new_queue;
> @@ -899,6 +1022,23 @@ static void btf_dump_emit_bit_padding(const struct btf_dump *d,
> }
> }
>
> +static void btf_dump_emit_decl_tags(struct btf_dump *d, __u32 id)
> +{
> + struct decl_tag_array *decl_tags = btf_dump_find_decl_tags(d, id);
> + struct btf_type *decl_tag_t;
> + const char *decl_tag_text;
> + __u32 i;
> +
> + if (!decl_tags)
> + return;
> +
> + for (i = 0; i < decl_tags->cnt; ++i) {
> + decl_tag_t = btf_type_by_id(d->btf, decl_tags->tag_ids[i]);
> + decl_tag_text = btf__name_by_offset(d->btf, decl_tag_t->name_off);
> + btf_dump_printf(d, " __attribute__((btf_decl_tag(\"%s\")))", decl_tag_text);
> + }
> +}
I'm wondering if we should anticipate that some compilers won't know
about btf_decl_tag attribute? It seems a bit off for btf_dump to worry
about this, but if we don't do something like:
#if __has_attribute(btf_decl_tag)
#define __btf_decl_tag(x) __attribute__((btf_decl_tag(#x)))
#else
#define __btf_decl_tag(x)
#endif
.
.
.
struct my_struct {
...
} __btf_decl_tag(awesomeness);
it will be hard for users to use resulting vmlinux.h with slightly older Clang?
> +
> static void btf_dump_emit_struct_fwd(struct btf_dump *d, __u32 id,
> const struct btf_type *t)
> {
> @@ -964,6 +1104,7 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
> btf_dump_printf(d, "%s}", pfx(lvl));
> if (packed)
> btf_dump_printf(d, " __attribute__((packed))");
> + btf_dump_emit_decl_tags(d, id);
> }
>
> static const char *missing_base_types[][2] = {
> --
> 2.34.1
>
