On Thu, Nov 10, 2022 at 6:43 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> Clang's `__attribute__((btf_decl_tag("...")))` is represented in BTF
> as a record of kind BTF_KIND_DECL_TAG with `type` field pointing to
> the type annotated with this attribute. This commit adds
> reconstitution of such attributes for BTF dump in C format.
>
> BTF doc says that BTF_KIND_DECL_TAGs should follow a target type but
> this is not enforced and tests don't honor this restriction.
> This commit uses hashmap to map types to the list of decl tags.
> The hashmap is filled incrementally by the function
> `btf_dump_assign_decl_tags` called from `btf_dump__dump_type` and
> `btf_dump__dump_type_data`.
>
> It is assumed that total number of types annotated with decl tags is
> relatively small, thus some space is saved by using hashmap instead of
> adding a new field to `struct btf_dump_type_aux_state`.
>
> It is assumed that list of decl tags associated with a single type is
> small. Thus the list is represented by an array which grows linearly.
>
> To accommodate older Clang versions decl tags are dumped using the
> following macro:
>
> #if __has_attribute(btf_decl_tag)
> #define __btf_decl_tag(x) __attribute__((btf_decl_tag(x)))
> #else
> #define __btf_decl_tag(x)
> #endif
>
> The macro definition is emitted upon first call to `btf_dump__dump_type`.
>
> Clang allows to attach btf_decl_tag attributes to the following kinds
> of items:
> - struct/union supported
> - struct/union field supported
> - typedef supported
> - global variables supported
> - function prototype parameters supported
> - function not applicable
> - function parameter not applicable
> - local variables not applicable
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> tools/lib/bpf/btf_dump.c | 181 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 173 insertions(+), 8 deletions(-)
>
[...]
> +/*
> + * 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)
> +{
> + __u32 id, new_cnt, type_cnt = btf__type_cnt(d->btf);
> + struct decl_tag_array *old_tags, *new_tags;
> + const struct btf_type *t;
> + size_t new_sz;
> + int err;
> +
> + for (id = d->next_decl_tag_scan_id; id < type_cnt; id++) {
> + t = btf__type_by_id(d->btf, id);
> + if (!btf_is_decl_tag(t))
> + continue;
> +
> + old_tags = NULL;
> + hashmap__find(d->decl_tags, t->type, &old_tags);
> + /* Assume small number of decl tags per id, increase array size by 1 */
> + new_cnt = old_tags ? old_tags->cnt + 1 : 1;
> + if (new_cnt == UINT_MAX)
> + return -ERANGE;
this can't happen, BTF IDs don't go up to UINT_MAX even, let's drop
unnecessary check
> + new_sz = sizeof(struct decl_tag_array) + new_cnt * sizeof(old_tags->tag_ids[0]);
> + new_tags = realloc(old_tags, new_sz);
> + if (!new_tags)
> + return -ENOMEM;
> +
> + new_tags->tag_ids[new_cnt - 1] = id;
> + new_tags->cnt = new_cnt;
> +
> + /* No need to update the map if realloc have not changed the pointer */
> + if (old_tags == new_tags)
> + continue;
this is a nice and simple optimization, I like it
> +
> + err = hashmap__set(d->decl_tags, t->type, new_tags, NULL, NULL);
> + if (!err)
> + continue;
> + /*
> + * If old_tags != NULL there is a record that holds it in the map, thus
> + * the hashmap__set() call should not fail as it does not have to
> + * allocate. If it does fail for some bizarre reason it's a bug and double
> + * free is imminent because of the previous realloc call.
> + */
> + if (old_tags)
> + pr_warn("hashmap__set() failed to update value for existing entry\n");
> + free(new_tags);
> + return err;
but this is an overkill, it should not fail and btf_dump is not the
place to log bugs in hashmap, please do just
(void)hashmap__set(...);
> + }
> +
> + d->next_decl_tag_scan_id = type_cnt;
> +
> + return 0;
> +}
> +
[...]
> 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?
> @@ -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.
> ret = btf_dump_dump_type_data(d, NULL, t, id, data, 0, 0);
>
> d->typed_dump = NULL;
> --
> 2.34.1
>
