[PATCH bpf-next v3 0/3] libbpf: btf_decl_tag attribute for btf dump in C format

[PATCH bpf-next v3 0/3] libbpf: btf_decl_tag attribute for btf dump in C format

[Eduard Zingerman]

Support for clang's __attribute__((btf_decl_tag("..."))) by
btf_dump__dump_type and btf_dump__dump_type_data functions.
Decl tag attributes are restored for:
- structs and unions
- struct and union fields
- typedefs
- global variables
- function prototype parameters

The attribute is restored using __btf_decl_tag macro that is printed
upon first call to btf_dump__dump_type function:

 #if __has_attribute(btf_decl_tag)
 #define __btf_decl_tag(x) __attribute__((btf_decl_tag(x)))
 #else
 #define __btf_decl_tag(x)
 #endif

To simplify testing of the btf_dump__dump_type_data the
prog_tests/btf_dump.c:test_btf_dump_case is extended to invoke
btf_dump__dump_type_data for each DATASEC object in the test case
binary file.

Changelog:
v2 -> v3:
- rebase to fix merge issues after recent hashmap interface update;
- call to btf_dump_assign_decl_tags removed from btf_dump__new as
  redundant.

v1 -> v2:
- prog_tests/btf_dump.c:test_btf_dump_case modified to print DATASECs
  using btf_dump__dump_type_data;
- support for decl tags applied to global variables and function
  prototype parameters;
- update to support interleaved calls to btf_dump__dump_type and
  btf__add_decl_tag (incremental dump);
- fix for potential double free error in btf_dump_assign_decl_tags;
- styling fixes suggested by Andrii.

RFC -> v1:
- support for decl tags applied to struct / union fields and typedefs;
- __btf_decl_tag macro;
- btf_dump->decl_tags hash and equal functions updated to use integer
  key instead of a pointer;
- realloc_decl_tags function removed;
- update for allocation logic in btf_dump_assign_decl_tags.

[v2]  https://lore.kernel.org/bpf/20221108153135.491383-1-eddyz87@gmail.com/
[v1]  https://lore.kernel.org/bpf/20221103134522.2764601-1-eddyz87@gmail.com/
[RFC] https://lore.kernel.org/bpf/20221025222802.2295103-4-eddyz87@gmail.com/

Eduard Zingerman (3):
  libbpf: __attribute__((btf_decl_tag("..."))) for btf dump in C format
  selftests/bpf: Dump data sections as part of btf_dump_test_case tests
  selftests/bpf: Tests for BTF_KIND_DECL_TAG dump in C format

 tools/lib/bpf/btf_dump.c                      | 181 +++++++++++++++-
 .../selftests/bpf/prog_tests/btf_dump.c       | 197 ++++++++++++++++--
 .../bpf/progs/btf_dump_test_case_decl_tag.c   |  65 ++++++
 3 files changed, 421 insertions(+), 22 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c

-- 
2.34.1

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

[Eduard Zingerman]

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@gmail.com>
---
 tools/lib/bpf/btf_dump.c | 181 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 173 insertions(+), 8 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 12f7039e0ab2..1cbc0da43cb4 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -75,6 +75,14 @@ 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 {
+	__u32 cnt;
+	__u32 tag_ids[0];
+};
+
 struct btf_dump {
 	const struct btf *btf;
 	btf_dump_printf_fn_t printf_fn;
@@ -111,6 +119,18 @@ 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;
+	/* indicates whether '#define __btf_decl_tag ...' was printed */
+	bool btf_decl_tag_is_defined;
+	/*
+	 * next id to be scanned for decl tags presence, needed to update
+	 * decl_tags map when dump_type calls are interleaved with BTF updates.
+	 */
+	__u32 next_decl_tag_scan_id;
 	/*
 	 * data for typed display; allocated if needed.
 	 */
@@ -127,6 +147,16 @@ static bool str_equal_fn(long a, long b, void *ctx)
 	return strcmp((void *)a, (void *)b) == 0;
 }
 
+static size_t identity_hash_fn(long key, void *ctx)
+{
+	return (size_t)key;
+}
+
+static bool identity_equal_fn(long k1, long k2, void *ctx)
+{
+	return k1 == k2;
+}
+
 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 +173,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,
@@ -170,15 +201,19 @@ struct btf_dump *btf_dump__new(const struct btf *btf,
 	d->type_names = hashmap__new(str_hash_fn, str_equal_fn, NULL);
 	if (IS_ERR(d->type_names)) {
 		err = PTR_ERR(d->type_names);
-		d->type_names = NULL;
 		goto err;
 	}
 	d->ident_names = hashmap__new(str_hash_fn, str_equal_fn, NULL);
 	if (IS_ERR(d->ident_names)) {
 		err = PTR_ERR(d->ident_names);
-		d->ident_names = NULL;
 		goto err;
 	}
+	d->decl_tags = hashmap__new(identity_hash_fn, identity_equal_fn, NULL);
+	if (IS_ERR(d->decl_tags)) {
+		err = PTR_ERR(d->decl_tags);
+		goto err;
+	}
+	d->next_decl_tag_scan_id = 1;
 
 	err = btf_dump_resize(d);
 	if (err)
@@ -219,13 +254,20 @@ static int btf_dump_resize(struct btf_dump *d)
 	return 0;
 }
 
-static void btf_dump_free_names(struct hashmap *map)
+static void btf_dump_free_strs_map(struct hashmap *map, bool free_keys, bool free_values)
 {
 	size_t bkt;
 	struct hashmap_entry *cur;
 
-	hashmap__for_each_entry(map, cur, bkt)
-		free((void *)cur->pkey);
+	if (IS_ERR_OR_NULL(map))
+		return;
+
+	hashmap__for_each_entry(map, cur, bkt) {
+		if (free_keys)
+			free((void *)cur->pkey);
+		if (free_values)
+			free(cur->pvalue);
+	}
 
 	hashmap__free(map);
 }
@@ -248,14 +290,16 @@ void btf_dump__free(struct btf_dump *d)
 	free(d->cached_names);
 	free(d->emit_queue);
 	free(d->decl_stack);
-	btf_dump_free_names(d->type_names);
-	btf_dump_free_names(d->ident_names);
+	btf_dump_free_strs_map(d->type_names, true, false);
+	btf_dump_free_strs_map(d->ident_names, true, false);
+	btf_dump_free_strs_map(d->decl_tags, false, true);
 
 	free(d);
 }
 
 static int btf_dump_order_type(struct btf_dump *d, __u32 id, bool through_ptr);
 static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id);
+static void btf_dump_ensure_btf_decl_tag_macro(struct btf_dump *d);
 
 /*
  * Dump BTF type in a compilable C syntax, including all the necessary
@@ -284,6 +328,9 @@ int btf_dump__dump_type(struct btf_dump *d, __u32 id)
 	if (err)
 		return libbpf_err(err);
 
+	btf_dump_ensure_btf_decl_tag_macro(d);
+	btf_dump_assign_decl_tags(d);
+
 	d->emit_queue_cnt = 0;
 	err = btf_dump_order_type(d, id, false);
 	if (err < 0)
@@ -373,6 +420,62 @@ static int btf_dump_mark_referenced(struct btf_dump *d)
 	return 0;
 }
 
+/*
+ * 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;
+		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;
+
+		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;
+	}
+
+	d->next_decl_tag_scan_id = type_cnt;
+
+	return 0;
+}
+
 static int btf_dump_add_emit_queue_id(struct btf_dump *d, __u32 id)
 {
 	__u32 *new_queue;
@@ -899,6 +1002,51 @@ static void btf_dump_emit_bit_padding(const struct btf_dump *d,
 	}
 }
 
+/*
+ * Define __btf_decl_tag to be either __attribute__ or noop.
+ */
+static void btf_dump_ensure_btf_decl_tag_macro(struct btf_dump *d)
+{
+	if (d->btf_decl_tag_is_defined || !hashmap__size(d->decl_tags))
+		return;
+
+	d->btf_decl_tag_is_defined = true;
+	btf_dump_printf(d, "#if __has_attribute(btf_decl_tag)\n");
+	btf_dump_printf(d, "#define __btf_decl_tag(x) __attribute__((btf_decl_tag(x)))\n");
+	btf_dump_printf(d, "#else\n");
+	btf_dump_printf(d, "#define __btf_decl_tag(x)\n");
+	btf_dump_printf(d, "#endif\n\n");
+}
+
+/*
+ * Emits a list of __btf_decl_tag(...) attributes attached to some type.
+ * Decl tags attached to a type and to it's fields reside in a same
+ * list, thus use component_idx to filter out relevant tags:
+ * - component_idx == -1 designates the type itself;
+ * - component_idx >=  0 designates specific field.
+ */
+static void btf_dump_emit_decl_tags(struct btf_dump *d,
+				    struct decl_tag_array *decl_tags,
+				    int component_idx)
+{
+	struct btf_type *t;
+	const char *text;
+	struct btf_decl_tag *tag;
+	__u32 i;
+
+	if (!decl_tags)
+		return;
+
+	for (i = 0; i < decl_tags->cnt; ++i) {
+		t = btf_type_by_id(d->btf, decl_tags->tag_ids[i]);
+		tag = btf_decl_tag(t);
+		if (tag->component_idx != component_idx)
+			continue;
+		text = btf__name_by_offset(d->btf, t->name_off);
+		btf_dump_printf(d, " __btf_decl_tag(\"%s\")", text);
+	}
+}
+
 static void btf_dump_emit_struct_fwd(struct btf_dump *d, __u32 id,
 				     const struct btf_type *t)
 {
@@ -914,11 +1062,13 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
 				     int lvl)
 {
 	const struct btf_member *m = btf_members(t);
+	struct decl_tag_array *decl_tags = NULL;
 	bool is_struct = btf_is_struct(t);
 	int align, i, packed, off = 0;
 	__u16 vlen = btf_vlen(t);
 
 	packed = is_struct ? btf_is_struct_packed(d->btf, id, t) : 0;
+	hashmap__find(d->decl_tags, id, &decl_tags);
 
 	btf_dump_printf(d, "%s%s%s {",
 			is_struct ? "struct" : "union",
@@ -945,6 +1095,7 @@ static void btf_dump_emit_struct_def(struct btf_dump *d,
 			m_sz = max((__s64)0, btf__resolve_size(d->btf, m->type));
 			off = m_off + m_sz * 8;
 		}
+		btf_dump_emit_decl_tags(d, decl_tags, i);
 		btf_dump_printf(d, ";");
 	}
 
@@ -964,6 +1115,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, decl_tags, -1);
 }
 
 static const char *missing_base_types[][2] = {
@@ -1090,6 +1242,7 @@ static void btf_dump_emit_typedef_def(struct btf_dump *d, __u32 id,
 				     const struct btf_type *t, int lvl)
 {
 	const char *name = btf_dump_ident_name(d, id);
+	struct decl_tag_array *decl_tags = NULL;
 
 	/*
 	 * Old GCC versions are emitting invalid typedef for __gnuc_va_list
@@ -1104,6 +1257,8 @@ static void btf_dump_emit_typedef_def(struct btf_dump *d, __u32 id,
 
 	btf_dump_printf(d, "typedef ");
 	btf_dump_emit_type_decl(d, t->type, name, lvl);
+	hashmap__find(d->decl_tags, id, &decl_tags);
+	btf_dump_emit_decl_tags(d, decl_tags, -1);
 }
 
 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
@@ -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);
+
 	ret = btf_dump_dump_type_data(d, NULL, t, id, data, 0, 0);
 
 	d->typed_dump = NULL;
-- 
2.34.1

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

[Andrii Nakryiko]

On Thu, Nov 10, 2022 at 6:43 AM Eduard Zingerman <eddyz87@gmail.com> 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@gmail.com>
> ---
>  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
>

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

[Eduard Zingerman]

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

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

[Andrii Nakryiko]

On Fri, Nov 11, 2022 at 1:30 PM Eduard Zingerman <eddyz87@gmail.com> 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@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:

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?

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

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

[Eduard Zingerman]

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

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

[Andrii Nakryiko]

On Tue, Nov 15, 2022 at 5:51 PM Eduard Zingerman <eddyz87@gmail.com> 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@gmail.com> 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@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:
> >
> > 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
> > > > >
> > >
>

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

[Yonghong Song]

On 11/11/22 1:30 PM, Eduard Zingerman 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@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?

Currently, btf decl tag is not supported to attach FUNC_PROTO 
parameters. We could add support in clang, do we have an actual use case
for this? if there is a use case, we can add support for it.

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

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

[Yonghong Song]

On 11/15/22 12:45 PM, Yonghong Song wrote:
> 
> 
> On 11/11/22 1:30 PM, Eduard Zingerman 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@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?
> 
> Currently, btf decl tag is not supported to attach FUNC_PROTO 
> parameters. We could add support in clang, do we have an actual use case
> for this? if there is a use case, we can add support for it.
> 
>>
>>>
>>>> @@ -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.

BTW, current use case of decl tag in the kernel is from Kumar's link 
list patch set:

https://lore.kernel.org/bpf/20221114191547.1694267-23-memxor@gmail.com/

+#define __contains(name, node) __attribute__((btf_decl_tag("contains:" 
#name ":" #node)))

to tag the struct member bpf_list_head as below:

struct foo {
	struct bpf_list_node node;
	int data;
};

struct map_value {
	struct bpf_list_head head __contains(foo, node);
};

https://lore.kernel.org/bpf/20221114191547.1694267-5-memxor@gmail.com/

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

[PATCH bpf-next v3 2/3] selftests/bpf: Dump data sections as part of btf_dump_test_case tests

[Eduard Zingerman]

Modify `test_btf_dump_case` to test `btf_dump__dump_type_data`
alongside `btf_dump__dump_type`.

The `test_btf_dump_case` function provides a convenient way to test
`btf_dump__dump_type` behavior as test cases are specified in separate
C files and any differences are reported using `diff` utility. This
commit extends `test_btf_dump_case` to call `btf_dump__dump_type_data`
for each `BTF_KIND_DATASEC` object in the test case object file.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../selftests/bpf/prog_tests/btf_dump.c       | 118 +++++++++++++++---
 1 file changed, 104 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index 24da335482d4..a0bdfc45660d 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
 #include <bpf/btf.h>
+#include <libelf.h>
+#include <gelf.h>
 
 static int duration = 0;
 
@@ -23,31 +25,104 @@ static struct btf_dump_test_case {
 	{"btf_dump: namespacing", "btf_dump_test_case_namespacing", false},
 };
 
-static int btf_dump_all_types(const struct btf *btf, void *ctx)
+static int btf_dump_all_types(const struct btf *btf, struct btf_dump *d)
 {
 	size_t type_cnt = btf__type_cnt(btf);
-	struct btf_dump *d;
 	int err = 0, id;
 
-	d = btf_dump__new(btf, btf_dump_printf, ctx, NULL);
-	err = libbpf_get_error(d);
-	if (err)
-		return err;
-
 	for (id = 1; id < type_cnt; id++) {
 		err = btf_dump__dump_type(d, id);
 		if (err)
-			goto done;
+			break;
+	}
+
+	return err;
+}
+
+/* Keep this as macro to retain __FILE__, __LINE__ values used by PRINT_FAIL */
+#define report_elf_error(fn)								\
+	({										\
+		int __err = elf_errno();						\
+		PRINT_FAIL("%s() failed %s(%d)\n", fn, elf_errmsg(__err), __err);	\
+		__err;									\
+	})
+
+static int btf_dump_datasec(Elf *elf, const struct btf *btf, struct btf_dump *d, __u32 id)
+{
+	const char *btf_sec, *elf_sec;
+	const struct btf_type *t;
+	Elf_Data *data = NULL;
+	Elf_Scn *scn = NULL;
+	size_t shstrndx;
+	GElf_Shdr sh;
+
+	if (elf_getshdrstrndx(elf, &shstrndx))
+		return report_elf_error("elf_getshdrstrndx");
+
+	t = btf__type_by_id(btf, id);
+	btf_sec = btf__str_by_offset(btf, t->name_off);
+
+	while ((scn = elf_nextscn(elf, scn)) != NULL) {
+		if (!gelf_getshdr(scn, &sh))
+			return report_elf_error("gelf_getshdr");
+		elf_sec = elf_strptr(elf, shstrndx, sh.sh_name);
+		if (!elf_sec)
+			return report_elf_error("elf_strptr");
+		if (strcmp(btf_sec, elf_sec) == 0) {
+			data = elf_getdata(scn, NULL);
+			if (!data)
+				return report_elf_error("elf_getdata");
+			break;
+		}
+	}
+
+	if (CHECK(!data, "btf_dump_datasec", "can't find ELF section %s\n", elf_sec))
+		return -1;
+
+	return btf_dump__dump_type_data(d, id, data->d_buf, data->d_size, NULL);
+}
+
+static int btf_dump_all_datasec(const struct btf *btf, struct btf_dump *d,
+				char *test_file, FILE *f)
+{
+	size_t type_cnt = btf__type_cnt(btf);
+	int err = 0, id, fd = 0;
+	Elf *elf = NULL;
+
+	fd = open(test_file, O_RDONLY | O_CLOEXEC);
+	if (CHECK(fd < 0, "open", "can't open %s for reading, %s(%d)\n",
+		  test_file, strerror(errno), errno)) {
+		err = errno;
+		goto done;
+	}
+
+	elf = elf_begin(fd, ELF_C_READ, NULL);
+	if (!elf) {
+		err = report_elf_error("elf_begin");
+		goto done;
+	}
+
+	for (id = 1; id < type_cnt; id++) {
+		if (!btf_is_datasec(btf__type_by_id(btf, id)))
+			continue;
+		err = btf_dump_datasec(elf, btf, d, id);
+		if (err)
+			break;
+		fprintf(f, "\n\n");
 	}
 
 done:
-	btf_dump__free(d);
+	if (fd)
+		close(fd);
+	if (elf)
+		elf_end(elf);
 	return err;
 }
 
 static int test_btf_dump_case(int n, struct btf_dump_test_case *t)
 {
 	char test_file[256], out_file[256], diff_cmd[1024];
+	struct btf_dump *d = NULL;
 	struct btf *btf = NULL;
 	int err = 0, fd = -1;
 	FILE *f = NULL;
@@ -86,12 +161,22 @@ static int test_btf_dump_case(int n, struct btf_dump_test_case *t)
 		goto done;
 	}
 
-	err = btf_dump_all_types(btf, f);
-	fclose(f);
-	close(fd);
-	if (CHECK(err, "btf_dump", "failure during C dumping: %d\n", err)) {
+	d = btf_dump__new(btf, btf_dump_printf, f, NULL);
+	err = libbpf_get_error(d);
+	if (CHECK(err, "btf_dump", "btf_dump__new failed: %d\n", err))
+		goto done;
+
+	err = btf_dump_all_types(btf, d);
+	if (CHECK(err, "btf_dump", "btf_dump_all_types failed: %d\n", err))
+		goto done;
+
+	err = btf_dump_all_datasec(btf, d, test_file, f);
+	if (CHECK(err, "btf_dump", "btf_dump_all_datasec failed: %d\n", err))
+		goto done;
+
+	if (CHECK(fflush(f), "btf_dump", "fflush() on %s failed: %s(%d)\n",
+		  test_file, strerror(errno), errno))
 		goto done;
-	}
 
 	snprintf(test_file, sizeof(test_file), "progs/%s.c", t->file);
 	if (access(test_file, R_OK) == -1)
@@ -122,6 +207,11 @@ static int test_btf_dump_case(int n, struct btf_dump_test_case *t)
 	remove(out_file);
 
 done:
+	if (f)
+		fclose(f);
+	if (fd >= 0)
+		close(fd);
+	btf_dump__free(d);
 	btf__free(btf);
 	return err;
 }
-- 
2.34.1

Re: [PATCH bpf-next v3 2/3] selftests/bpf: Dump data sections as part of btf_dump_test_case tests

[Andrii Nakryiko]

On Thu, Nov 10, 2022 at 6:43 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Modify `test_btf_dump_case` to test `btf_dump__dump_type_data`
> alongside `btf_dump__dump_type`.
>
> The `test_btf_dump_case` function provides a convenient way to test
> `btf_dump__dump_type` behavior as test cases are specified in separate
> C files and any differences are reported using `diff` utility. This
> commit extends `test_btf_dump_case` to call `btf_dump__dump_type_data`
> for each `BTF_KIND_DATASEC` object in the test case object file.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---

please use ASSERT_xxx() instead of CHECK()

>  .../selftests/bpf/prog_tests/btf_dump.c       | 118 +++++++++++++++---
>  1 file changed, 104 insertions(+), 14 deletions(-)
>

[...]

[PATCH bpf-next v3 3/3] selftests/bpf: Tests for BTF_KIND_DECL_TAG dump in C format

[Eduard Zingerman]

Covers the following cases:
- `__atribute__((btf_decl_tag("...")))` could be applied to structs
  and unions;
- decl tag applied to an empty struct is printed on a single line;
- decl tags with the same name could be applied to several structs;
- several decl tags could be applied to the same struct;
- attribute `packed` works fine with decl tags (it is a separate
  branch in `tools/lib/bpf/btf_dump.c:btf_dump_emit_attributes`;
- decl tag could be attached to typedef;
- decl tag could be attached to a struct field;
- decl tag could be attached to a struct field and a struct itself
  simultaneously;
- decl tag could be attached to a global variable;
- decl tag could be attached to a func proto parameter;
- btf__add_decl_tag could be interleaved with btf_dump__dump_type calls.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../selftests/bpf/prog_tests/btf_dump.c       | 79 +++++++++++++++++++
 .../bpf/progs/btf_dump_test_case_decl_tag.c   | 65 +++++++++++++++
 2 files changed, 144 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c

diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index a0bdfc45660d..2e6135a555e3 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -23,6 +23,7 @@ static struct btf_dump_test_case {
 	{"btf_dump: bitfields", "btf_dump_test_case_bitfields", true},
 	{"btf_dump: multidim", "btf_dump_test_case_multidim", false},
 	{"btf_dump: namespacing", "btf_dump_test_case_namespacing", false},
+	{"btf_dump: decl_tag", "btf_dump_test_case_decl_tag", true},
 };
 
 static int btf_dump_all_types(const struct btf *btf, struct btf_dump *d)
@@ -345,6 +346,82 @@ static void test_btf_dump_incremental(void)
 	btf__free(btf);
 }
 
+static void test_btf_dump_func_proto_decl_tag(void)
+{
+	struct btf *btf = NULL;
+	struct btf_dump *d = NULL;
+	int id, err;
+
+	dump_buf_file = open_memstream(&dump_buf, &dump_buf_sz);
+	if (!ASSERT_OK_PTR(dump_buf_file, "dump_memstream"))
+		return;
+	btf = btf__new_empty();
+	if (!ASSERT_OK_PTR(btf, "new_empty"))
+		goto err_out;
+	d = btf_dump__new(btf, btf_dump_printf, dump_buf_file, NULL);
+	if (!ASSERT_OK(libbpf_get_error(d), "btf_dump__new"))
+		goto err_out;
+
+	/* First, BTF corresponding to the following C code:
+	 *
+	 * typedef void (*fn)(int a __btf_decl_tag("a_tag"));
+	 *
+	 */
+	id = btf__add_int(btf, "int", 4, BTF_INT_SIGNED);
+	ASSERT_EQ(id, 1, "int_id");
+	id = btf__add_func_proto(btf, 0);
+	ASSERT_EQ(id, 2, "func_proto_id");
+	err = btf__add_func_param(btf, "a", 1);
+	ASSERT_OK(err, "func_param_ok");
+	id = btf__add_decl_tag(btf, "a_tag", 2, 0);
+	ASSERT_EQ(id, 3, "decl_tag_a");
+	id = btf__add_ptr(btf, 2);
+	ASSERT_EQ(id, 4, "proto_ptr");
+	id = btf__add_typedef(btf, "fn", 4);
+	ASSERT_EQ(id, 5, "typedef");
+
+	err = btf_dump_all_types(btf, d);
+	ASSERT_OK(err, "btf_dump_all_types #1");
+	fflush(dump_buf_file);
+	dump_buf[dump_buf_sz] = 0; /* some libc implementations don't do this */
+
+	ASSERT_STREQ(dump_buf,
+		     "#if __has_attribute(btf_decl_tag)\n"
+		     "#define __btf_decl_tag(x) __attribute__((btf_decl_tag(x)))\n"
+		     "#else\n"
+		     "#define __btf_decl_tag(x)\n"
+		     "#endif\n"
+		     "\n"
+		     "typedef void (*fn)(int a __btf_decl_tag(\"a_tag\"));\n\n",
+		     "decl tags for fn");
+
+	/* Next, add BTF corresponding to the following C code:
+	 *
+	 * typedef int foo __btf_decl_tag("foo_tag");
+	 *
+	 * To verify that decl_tag's table is updated incrementally.
+	 */
+	id = btf__add_typedef(btf, "foo", 1);
+	ASSERT_EQ(id, 6, "typedef");
+	id = btf__add_decl_tag(btf, "foo_tag", 6, -1);
+	ASSERT_EQ(id, 7, "decl_tag_foo");
+
+	fseek(dump_buf_file, 0, SEEK_SET);
+	err = btf_dump_all_types(btf, d);
+	ASSERT_OK(err, "btf_dump_all_types #2");
+	fflush(dump_buf_file);
+	dump_buf[dump_buf_sz] = 0; /* some libc implementations don't do this */
+
+	ASSERT_STREQ(dump_buf,
+		     "typedef int foo __btf_decl_tag(\"foo_tag\");\n\n",
+		     "decl tags for foo");
+err_out:
+	fclose(dump_buf_file);
+	free(dump_buf);
+	btf_dump__free(d);
+	btf__free(btf);
+}
+
 #define STRSIZE				4096
 
 static void btf_dump_snprintf(void *ctx, const char *fmt, va_list args)
@@ -963,6 +1040,8 @@ void test_btf_dump() {
 	}
 	if (test__start_subtest("btf_dump: incremental"))
 		test_btf_dump_incremental();
+	if (test__start_subtest("btf_dump: func arg decl_tag"))
+		test_btf_dump_func_proto_decl_tag();
 
 	btf = libbpf_find_kernel_btf();
 	if (!ASSERT_OK_PTR(btf, "no kernel BTF found"))
diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c
new file mode 100644
index 000000000000..1205351e9a68
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+
+/*
+ * BTF-to-C dumper test for __atribute__((btf_decl_tag("..."))).
+ */
+
+#define SEC(x) __attribute__((section(x)))
+
+/* ----- START-EXPECTED-OUTPUT ----- */
+#if __has_attribute(btf_decl_tag)
+#define __btf_decl_tag(x) __attribute__((btf_decl_tag(x)))
+#else
+#define __btf_decl_tag(x)
+#endif
+
+struct empty_with_tag {} __btf_decl_tag("a");
+
+struct one_tag {
+	int x;
+} __btf_decl_tag("b");
+
+struct same_tag {
+	int x;
+} __btf_decl_tag("b");
+
+struct two_tags {
+	int x;
+} __btf_decl_tag("a") __btf_decl_tag("b");
+
+struct packed {
+	int x;
+	short y;
+} __attribute__((packed)) __btf_decl_tag("another_name");
+
+typedef int td_with_tag __btf_decl_tag("td");
+
+struct tags_on_fields {
+	int x __btf_decl_tag("t1");
+	int y;
+	int z __btf_decl_tag("t2") __btf_decl_tag("t3");
+};
+
+struct tag_on_field_and_struct {
+	int x __btf_decl_tag("t1");
+} __btf_decl_tag("t2");
+
+struct root_struct {
+	struct empty_with_tag a;
+	struct one_tag b;
+	struct same_tag c;
+	struct two_tags d;
+	struct packed e;
+	td_with_tag f;
+	struct tags_on_fields g;
+	struct tag_on_field_and_struct h;
+};
+
+SEC(".data") int global_var __btf_decl_tag("var_tag") = (int)777;
+
+/* ------ END-EXPECTED-OUTPUT ------ */
+
+int f(struct root_struct *s)
+{
+	return 0;
+}
-- 
2.34.1

Re: [PATCH bpf-next v3 3/3] selftests/bpf: Tests for BTF_KIND_DECL_TAG dump in C format

[Andrii Nakryiko]

On Thu, Nov 10, 2022 at 6:43 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Covers the following cases:
> - `__atribute__((btf_decl_tag("...")))` could be applied to structs
>   and unions;
> - decl tag applied to an empty struct is printed on a single line;
> - decl tags with the same name could be applied to several structs;
> - several decl tags could be applied to the same struct;
> - attribute `packed` works fine with decl tags (it is a separate
>   branch in `tools/lib/bpf/btf_dump.c:btf_dump_emit_attributes`;
> - decl tag could be attached to typedef;
> - decl tag could be attached to a struct field;
> - decl tag could be attached to a struct field and a struct itself
>   simultaneously;
> - decl tag could be attached to a global variable;
> - decl tag could be attached to a func proto parameter;
> - btf__add_decl_tag could be interleaved with btf_dump__dump_type calls.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/btf_dump.c       | 79 +++++++++++++++++++
>  .../bpf/progs/btf_dump_test_case_decl_tag.c   | 65 +++++++++++++++
>  2 files changed, 144 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c
>

[...]

> +       /* First, BTF corresponding to the following C code:
> +        *
> +        * typedef void (*fn)(int a __btf_decl_tag("a_tag"));
> +        *
> +        */
> +       id = btf__add_int(btf, "int", 4, BTF_INT_SIGNED);
> +       ASSERT_EQ(id, 1, "int_id");
> +       id = btf__add_func_proto(btf, 0);
> +       ASSERT_EQ(id, 2, "func_proto_id");
> +       err = btf__add_func_param(btf, "a", 1);
> +       ASSERT_OK(err, "func_param_ok");
> +       id = btf__add_decl_tag(btf, "a_tag", 2, 0);
> +       ASSERT_EQ(id, 3, "decl_tag_a");
> +       id = btf__add_ptr(btf, 2);
> +       ASSERT_EQ(id, 4, "proto_ptr");
> +       id = btf__add_typedef(btf, "fn", 4);
> +       ASSERT_EQ(id, 5, "typedef");

can you please also add decl_tag for func_proto itself (comp_idx == -1)

> +
> +       err = btf_dump_all_types(btf, d);
> +       ASSERT_OK(err, "btf_dump_all_types #1");
> +       fflush(dump_buf_file);
> +       dump_buf[dump_buf_sz] = 0; /* some libc implementations don't do this */
> +
> +       ASSERT_STREQ(dump_buf,
> +                    "#if __has_attribute(btf_decl_tag)\n"
> +                    "#define __btf_decl_tag(x) __attribute__((btf_decl_tag(x)))\n"
> +                    "#else\n"
> +                    "#define __btf_decl_tag(x)\n"
> +                    "#endif\n"
> +                    "\n"
> +                    "typedef void (*fn)(int a __btf_decl_tag(\"a_tag\"));\n\n",
> +                    "decl tags for fn");
> +

[...]

> +struct tag_on_field_and_struct {
> +       int x __btf_decl_tag("t1");
> +} __btf_decl_tag("t2");
> +
> +struct root_struct {
> +       struct empty_with_tag a;
> +       struct one_tag b;
> +       struct same_tag c;
> +       struct two_tags d;
> +       struct packed e;
> +       td_with_tag f;
> +       struct tags_on_fields g;
> +       struct tag_on_field_and_struct h;
> +};
> +
> +SEC(".data") int global_var __btf_decl_tag("var_tag") = (int)777;

do you need explicit SEC(".data")? I'd expect global_var is put into
.data anyways. If it's about __attribute__((unused)), then we can do
that more explicitly here instead of through SEC()? Or just make sure
global_var is used in f() internally

> +
> +/* ------ END-EXPECTED-OUTPUT ------ */
> +
> +int f(struct root_struct *s)
> +{
> +       return 0;
> +}
> --
> 2.34.1
>