dwarves.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH dwarves v2 0/5] Support for new btf_type_tag encoding
@ 2023-03-14 23:04 Eduard Zingerman
  2023-03-14 23:04 ` [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute Eduard Zingerman
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-14 23:04 UTC (permalink / raw)
  To: dwarves, arnaldo.melo
  Cc: bpf, kernel-team, ast, daniel, andrii, yhs, jose.marchesi,
	david.faust, alan.maguire, Eduard Zingerman

In recent discussion in BPF mailing list ([1], look for Solution #2)
participants agreed to add a new DWARF representation for
"btf_type_tag" annotations.

Existing representation is DW_TAG_LLVM_annotation object attached as a
child to a DW_TAG_pointer_type. It means that "btf_type_tag"
annotation is attached to a pointee type.

New representation is DW_TAG_LLVM_annotation object attached as a
child to *any* type. It means that "btf_type_tag" annotation is
attached to the parent type.

Here is an example:

    int __attribute__((btf_type_tag("tag1"))) *g;

And corresponding DWARF:

    0x0000001e:   DW_TAG_variable
                    DW_AT_name      ("g")
                    DW_AT_type      (0x00000029 "int *")

    0x00000029:   DW_TAG_pointer_type
                    DW_AT_type      (0x00000032 "int")

    0x0000002e:     DW_TAG_LLVM_annotation
                      DW_AT_name    ("btf_type_tag")
                      DW_AT_const_value     ("tag1")

    0x00000032:   DW_TAG_base_type
                    DW_AT_name      ("int")


This patch adds logic necessary to handle such annotations in the
pahole tool. Examples like below are supported:

  #define __tag(val) __attribute__((btf_type_tag("__" #val)))

  struct      alpha {};
  union       bravo {};
  enum        charlie { X };
  typedef int delta;

  struct echo {
    int          * __tag(a)  a;
    int            __tag(b) *b;
    int            __tag(c)  c;
    void           __tag(d) *d;
    void           __tag(e) *e;
    struct alpha   __tag(f)  f;
    union  bravo   __tag(g)  g;
    enum   charlie __tag(h)  h;
    delta          __tag(i)  i;
    int __tag(j_result) (__tag(j) *j)(int __tag(j_param));
  } g;

Implementation details
----------------------

Although this was not discussed in the mailing list, the proposed
implementation acts in a following way (for compatibility reasons):
- both forms could be present in the debug info;
- if any annotations corresponding to the new form are present in the
  debug info, annotations corresponding to the old form are ignored.

Because the new form of type tags could be applied to any type it is
somewhat invasive from implementation point of view:
- Types `struct btf_type_tag_type` and `struct llvm_annotation` are
  consolidated as a single type `struct llvm_annotation`, in order to
  reside in a single `annots` list associated with struct/union/enum
  or typedef.
- The `annots` list, used to hold references to annotation objects,
  is put directly in `struct tag`.
- At the load phase it is not yet known whether new or old form is
  used for type tags encoding, so `struct llvm_annotation` objects are
  not added to the types table.
- The recode phase now consists of several steps:
  - depending on the results of the load phase, `struct llvm_annotation`
    objects corresponding to either old or new representations are
    added to the types table;
  - `tag__recode_dwarf_type()` is executed as usual, but it also
    collects information about type tags;
  - references to types that have type tag annotations are updated to
    point to the first annotation object.

Corresponding clang changes are tracked in [6].

Testing
-------

To verify the changes I used the following:
- Tools:
  - "LLVM-main"   :: LLVM at revision [3];
  - "LLVM-new"    :: LLVM at revision [3] with patches [6] applied;
  - "gcc"         :: GCC version 11.3 (no support for btf_type_tag annotations);
  - "pahole-next" :: dwarves at revision [4];
  - "pahole-new"  :: dwarves at revision [4] + this patch,
  - "kernel"      :: Linux Kernel bpf-next branch at revision [5]
- test cases:
  - kernel build;
  - kernel BPF test cases build, BPF tests execution
    (test_verifier, test_progs, test_progs-no_alu32, test_maps);
  - btfdiff script (suggested by Arnaldo, [2]).
- tool combinations (kernel compiler / clang for BPF tests / pahole version):
  - LLVM-main / LLVM-main / pahole-new
    - kernel build : ok
    - bpf tests    : ok
    - btfdiff      : ok (modulo diff #1, see below)
  - gcc       / LLVM-main / pahole-new
    - kernel build : ok
    - bpf tests    : ok
    - btfdiff      : ok
  - LLVM-new  / LLVM-new  / pahole-next
    - kernel build : ok with warnings (see warn #1 below)
    - bpf tests    : ok
    - btfdiff      : fails (see diff #2 below)
  - LLVM-new  / LLVM-new  / pahole-new
    - kernel build : ok
    - bpf tests    : ok
    - btfdiff      : ok (modulo diff #1, see below)
  - gcc       / LLVM-new  / pahole-new
    - kernel build : ok
    - bpf tests    : ok
    - btfdiff      : ok

Note on BPF tests: test case `verif_scale_loop6` fails for all
configurations above and 'LLVM-main / LLVM-main / pahole-next',
thus I consider this issue as unrelated.

Diff #1: Difference in flexible printing, several occurrences as below:

  @ -10531,7 +10531,7 @ struct bpf_cand_cache {
          struct {
                  const struct btf  * btf;                 /*    16     8 */
                  u32                id;                   /*    24     4 */
  -       } cands[0]; /*    16     0 */
  +       } cands[]; /*    16     0 */
   
          /* size: 16, cachelines: 1, members: 5 */
          /* last cacheline: 16 bytes */

Diff #2: pahole-next does not know how to print type name for types
with type tags, when reading from BTF:

  @ -72998,8 +73022,8 @ struct sock {
          /* --- cacheline 19 boundary (1216 bytes) --- */
          int                        (*sk_backlog_rcv)(struct sock *, struct sk_buff *); /*  1216     8 */
          void                       (*sk_destruct)(struct sock *); /*  1224     8 */
  -       rcu *                      sk_reuseport_cb;      /*  1232     8 */
  -       rcu *                      sk_bpf_storage;       /*  1240     8 */
  +       <ERROR                    > sk_reuseport_cb;     /*  1232     8 */
  +       <ERROR                    > sk_bpf_storage;      /*  1240     8 */

(Also DWARF names refer to TYPE_TAG, not actual type name, fixed in pahole-new).

Warn #1: pahole-next complains about unexpected child tags generated
by clang, e.g.:

  die__create_new_tag: unspecified_type WITH children!
  die__create_new_base_type: DW_TAG_base_type WITH children!


Performance impact
------------------

The update to `struct tag` might raise concerns regarding memory
usage, additional steps in recode phase might raise concerns regarding
execution time. Below is statistics collected for Kernel BTF
generation.

LLVM-new / LLVM-new / pahole-new:

$ /usr/bin/time -v pahole -J --btf_gen_floats -j --lang_exclude=rust .tmp_vmlinux.btf
    ...
	User time (seconds): 22.29
	System time (seconds): 0.47
	Percent of CPU this job got: 483%
    ...
	Maximum resident set size (kbytes): 714524
    ...

LLVM-new / LLVM-new / pahole-next:

$ /usr/bin/time -v pahole -J --btf_gen_floats -j --lang_exclude=rust .tmp_vmlinux.btf
    ...
	User time (seconds): 20.96
	System time (seconds): 0.44
	Percent of CPU this job got: 473%
    ...
	Maximum resident set size (kbytes): 700848
    ...

Changelog
---------

V1 -> V2:
- The patch is split in 5 parts to (hopefully) simplify the review:
  - #1, #2: two simple patches for fprintf and btf_loader to fix printing
    issue for types annotated by BTF type tags;
  - #3:  merges `struct llvm_annotation` and `struct btf_type_tag_type`
    as a preparatory step;
  - #4: introduces `struct unspecified_type` as a preparatory step;
  - #5: main logic for `btf:type_tag` support, this once can't
    be split further w/o parts loosing some functionality for kernel
    build and/or bpf tests.
- `reallocarray()` in `push_btf_type_tag_mapping()` is replaced by
  `realloc()` (suggested by Alan);
- The sequence `free(dcu->hash_tags); free(dcu->hash_types);` added in
  V1 is removed from `dwarf_cu__delete()`. It was a fix for some
  valgrind errors reported for `pahole -F dwarf`, but this is
  unrelated and the fix is incomplete.

Links & revisions
-----------------

[1] Mailing list discussion regarding `btf:type_tag`
    Various approaches are discussed, Solution #2 is accepted
    https://lore.kernel.org/bpf/87r0w9jjoq.fsf@oracle.com/
[2] Suggestion to use btfdiff
    https://lore.kernel.org/dwarves/ZAKpZGSHTvsS4r8E@kernel.org/T/#mddbfe661e339485fb2b0e706b31329b46bf61bda
[3] f759275c1c8e ("[AMDGPU] Regenerate sdwa-peephole.ll")
[4] a9498899109d ("dwarf_loader: Support for btf:type_tag")
[5] 49b5300f1f8f ("Merge branch 'Support stashing local kptrs with bpf_kptr_xchg'")
[6] LLVM changes to generate btf:type_tag, revisions stack:
    https://reviews.llvm.org/D143966
    https://reviews.llvm.org/D143967
    https://reviews.llvm.org/D145891
[7] V1 of this patch-set
    https://lore.kernel.org/dwarves/2232e368e55eb401bde45ce1b20fb710e379ae9c.camel@gmail.com/T/

Eduard Zingerman (5):
  fprintf: Correct names for types with btf_type_tag attribute
  btf_loader: A hack for BTF import of btf_type_tag attributes
  dwarf_loader: Consolidate llvm_annotation and btf_type_tag_type
  dwarf_loader: Track unspecified types in a separate list
  dwarf_loader: Support for btf:type_tag

 btf_encoder.c     |  13 +-
 btf_loader.c      |  15 +-
 dwarf_loader.c    | 760 +++++++++++++++++++++++++++++++++++++---------
 dwarves.c         |   1 +
 dwarves.h         |  64 ++--
 dwarves_fprintf.c |  13 +
 6 files changed, 686 insertions(+), 180 deletions(-)

-- 
2.39.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-14 23:04 [PATCH dwarves v2 0/5] Support for new btf_type_tag encoding Eduard Zingerman
@ 2023-03-14 23:04 ` Eduard Zingerman
  2023-03-27 11:46   ` Arnaldo Carvalho de Melo
  2023-03-28 13:59   ` Arnaldo Carvalho de Melo
  2023-03-14 23:04 ` [PATCH dwarves v2 2/5] btf_loader: A hack for BTF import of btf_type_tag attributes Eduard Zingerman
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-14 23:04 UTC (permalink / raw)
  To: dwarves, arnaldo.melo
  Cc: bpf, kernel-team, ast, daniel, andrii, yhs, jose.marchesi,
	david.faust, alan.maguire, Eduard Zingerman

The following example contains a structure field annotated with
btf_type_tag attribute:

    #define __tag1 __attribute__((btf_type_tag("tag1")))

    struct st {
      int __tag1 *a;
    } g;

It is not printed correctly by `pahole -F dwarf` command:

    $ clang -g -c test.c -o test.o
    pahole -F dwarf test.o
    struct st {
    	tag1 *                     a;                    /*     0     8 */
    	...
    };

Note the type for variable `a`: `tag1` is printed instead of `int`.
This commit teaches `type__fprintf()` and `__tag_name()` logic to skip
`DW_TAG_LLVM_annotation` objects that are used to encode type tags.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 dwarves_fprintf.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index e8399e7..1e6147a 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -572,6 +572,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
 	case DW_TAG_restrict_type:
 	case DW_TAG_atomic_type:
 	case DW_TAG_unspecified_type:
+	case DW_TAG_LLVM_annotation:
 		type = cu__type(cu, tag->type);
 		if (type == NULL && tag->type != 0)
 			tag__id_not_found_snprintf(bf, len, tag->type);
@@ -786,6 +787,10 @@ next_type:
 			n = tag__has_type_loop(type, ptype, NULL, 0, fp);
 			if (n)
 				return printed + n;
+			if (ptype->tag == DW_TAG_LLVM_annotation) {
+				type = ptype;
+				goto next_type;
+			}
 			if (ptype->tag == DW_TAG_subroutine_type) {
 				printed += ftype__fprintf(tag__ftype(ptype),
 							  cu, name, 0, 1,
@@ -880,6 +885,14 @@ print_modifier: {
 		else
 			printed += enumeration__fprintf(type, &tconf, fp);
 		break;
+	case DW_TAG_LLVM_annotation: {
+		struct tag *ttype = cu__type(cu, type->type);
+		if (ttype) {
+			type = ttype;
+			goto next_type;
+		}
+		goto out_type_not_found;
+	}
 	}
 out:
 	if (type_expanded)
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH dwarves v2 2/5] btf_loader: A hack for BTF import of btf_type_tag attributes
  2023-03-14 23:04 [PATCH dwarves v2 0/5] Support for new btf_type_tag encoding Eduard Zingerman
  2023-03-14 23:04 ` [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute Eduard Zingerman
@ 2023-03-14 23:04 ` Eduard Zingerman
  2023-03-14 23:04 ` [PATCH dwarves v2 3/5] dwarf_loader: Consolidate llvm_annotation and btf_type_tag_type Eduard Zingerman
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-14 23:04 UTC (permalink / raw)
  To: dwarves, arnaldo.melo
  Cc: bpf, kernel-team, ast, daniel, andrii, yhs, jose.marchesi,
	david.faust, alan.maguire, Eduard Zingerman

Pahole does not print type names correctly for the following example,
when BTF loader is used:

    #define __tag1 __attribute__((btf_type_tag("tag1")))

    struct st {
      int __tag1 *a;
    } g;

Here is the pahole output:

    $ clang -target bpf -g -c test.c -o test.o
    $ pahole -F btf test.o
    BTF: idx: 2, Unknown kind 18
    struct st {
    	<ERROR                    > a;                   /*     0     8 */
        ...
    };

Note the type name for field `a`.

This commit adds a workaround for this issue: it creates a tag
instance with tag->tag set to `DW_TAG_LLVM_annotation` and `tag->type`
pointing to the type wrapped by `BTF_KIND_TYPE_TAG`, `int` for the
example above.

Note that this is not a complete replication of behavior of DWARF
loader. When DWARF is processed type tag instances are added to the
annotations list of the parent pointer type. However, this is
sufficient to fix the printing issue and helps with `btfdiff` script.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 btf_loader.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/btf_loader.c b/btf_loader.c
index e579323..3fe07d0 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -429,10 +429,11 @@ static int create_new_tag(struct cu *cu, int type, const struct btf_type *tp, ui
 		return -ENOMEM;
 
 	switch (type) {
-	case BTF_KIND_CONST:	tag->tag = DW_TAG_const_type;	 break;
-	case BTF_KIND_PTR:	tag->tag = DW_TAG_pointer_type;  break;
-	case BTF_KIND_RESTRICT:	tag->tag = DW_TAG_restrict_type; break;
-	case BTF_KIND_VOLATILE:	tag->tag = DW_TAG_volatile_type; break;
+	case BTF_KIND_CONST:	tag->tag = DW_TAG_const_type;	   break;
+	case BTF_KIND_PTR:	tag->tag = DW_TAG_pointer_type;    break;
+	case BTF_KIND_RESTRICT:	tag->tag = DW_TAG_restrict_type;   break;
+	case BTF_KIND_VOLATILE:	tag->tag = DW_TAG_volatile_type;   break;
+	case BTF_KIND_TYPE_TAG:	tag->tag = DW_TAG_LLVM_annotation; break;
 	default:
 		free(tag);
 		printf("%s: Unknown type %d\n\n", __func__, type);
@@ -489,6 +490,12 @@ static int btf__load_types(struct btf *btf, struct cu *cu)
 		case BTF_KIND_PTR:
 		case BTF_KIND_CONST:
 		case BTF_KIND_RESTRICT:
+		/* For type tag it's a bit of a lie.
+		 * In DWARF it is encoded as a child tag of whatever type it
+		 * applies to. Here we load it as a standalone tag with a pointer
+		 * to a next type only to have a valid ID in the types table.
+		 */
+		case BTF_KIND_TYPE_TAG:
 			err = create_new_tag(cu, type, type_ptr, type_index);
 			break;
 		case BTF_KIND_UNKN:
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH dwarves v2 3/5] dwarf_loader: Consolidate llvm_annotation and btf_type_tag_type
  2023-03-14 23:04 [PATCH dwarves v2 0/5] Support for new btf_type_tag encoding Eduard Zingerman
  2023-03-14 23:04 ` [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute Eduard Zingerman
  2023-03-14 23:04 ` [PATCH dwarves v2 2/5] btf_loader: A hack for BTF import of btf_type_tag attributes Eduard Zingerman
@ 2023-03-14 23:04 ` Eduard Zingerman
  2023-03-14 23:04 ` [PATCH dwarves v2 4/5] dwarf_loader: Track unspecified types in a separate list Eduard Zingerman
  2023-03-14 23:04 ` [PATCH dwarves v2 5/5] dwarf_loader: Support for btf:type_tag Eduard Zingerman
  4 siblings, 0 replies; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-14 23:04 UTC (permalink / raw)
  To: dwarves, arnaldo.melo
  Cc: bpf, kernel-team, ast, daniel, andrii, yhs, jose.marchesi,
	david.faust, alan.maguire, Eduard Zingerman

In recent discussion in BPF mailing list ([1]) participants agreed to
add a new DWARF representation for "btf_type_tag" annotations.

Existing representation is DW_TAG_LLVM_annotation object attached as a
child to a DW_TAG_pointer_type. It means that "btf_type_tag"
annotation is attached to a pointee type.

New representation is DW_TAG_LLVM_annotation object attached as a
child to *any* type. It means that "btf_type_tag" annotation is
attached to the parent type.

For example, consider the following C code:

    struct alpha {
      int __attribute__((btf_type_tag("__alpha_a"))) *a;
    } g;

And corresponding DWARF:

0x29:   DW_TAG_structure_type
          DW_AT_name      ("alpha")

0x2e:     DW_TAG_member
            DW_AT_name    ("a")
            DW_AT_type    (0x38 "int *")

0x38:   DW_TAG_pointer_type
          DW_AT_type      (0x41 "int")

0x3d:     DW_TAG_LLVM_annotation
            DW_AT_name    ("btf_type_tag")
            DW_AT_const_value     ("__alpha_a")
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
          Old style representation

0x41:   DW_TAG_base_type
          DW_AT_name      ("int")
          DW_AT_encoding  (DW_ATE_signed)
          DW_AT_byte_size (0x04)

0x45:     DW_TAG_LLVM_annotation
            DW_AT_name    ("btf:type_tag")
            DW_AT_const_value     ("__alpha_a")
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
          New style representation

This means that new style type tags could be attached to any type from
the list below:
- base types;
- arrays;
- pointers;
- structs
- unions;
- enums;
- typedefs.

This commit is a preparatory step for `btf:type_tag` support:
- structs, unions and typedefs could be annotated with two kinds of
  `DW_TAG_LLVM_annotation` when new type tag representation is used:
  - BTF_DECL_TAG
  - BTF_TYPE_TAG
  In order to keep these objects in a single annotations list
  `struct llvm_annotation` and `struct btf_type_tag_type` are
  consolidated as a single type with a special discriminator
  field to distinguish one from the other;
- Because many types could be annotated with `btf:type_tag` the `annots`
  field is moved to `struct tag`, consequently:
  - type `struct btf_type_tag_type_ptr` is removed;
  - field `struct namespace::annots` is removed.

[1] Mailing list discussion regarding `btf:type_tag`
    Various approaches are discussed, Solution #2 is accepted
    https://lore.kernel.org/bpf/87r0w9jjoq.fsf@oracle.com/

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 btf_encoder.c  |  13 ++-
 dwarf_loader.c | 230 ++++++++++++++++++++++++++-----------------------
 dwarves.h      |  49 ++++-------
 3 files changed, 150 insertions(+), 142 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 65f6e71..1aa4ffc 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -893,6 +893,9 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct functio
 		return -1;
 	}
 	list_for_each_entry(annot, &fn->annots, node) {
+		if (annot->kind != BTF_DECL_TAG)
+			continue;
+
 		tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id,
 							annot->component_idx);
 		if (tag_type_id < 0) {
@@ -1175,7 +1178,7 @@ static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
 		name = namespace__name(tag__namespace(tag));
 		return btf_encoder__add_ref_type(encoder, BTF_KIND_TYPEDEF, ref_type_id, name, false);
 	case DW_TAG_LLVM_annotation:
-		name = tag__btf_type_tag(tag)->value;
+		name = tag__llvm_annotation(tag)->value;
 		return btf_encoder__add_ref_type(encoder, BTF_KIND_TYPE_TAG, ref_type_id, name, false);
 	case DW_TAG_structure_type:
 	case DW_TAG_union_type:
@@ -1600,6 +1603,9 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
 		}
 
 		list_for_each_entry(annot, &var->annots, node) {
+			if (annot->kind != BTF_DECL_TAG)
+				continue;
+
 			int tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, id, annot->component_idx);
 			if (tag_type_id < 0) {
 				fprintf(stderr, "error: failed to encode tag '%s' to variable '%s' with component_idx %d\n",
@@ -1793,7 +1799,10 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 
 		btf_type_id = encoder->type_id_off + core_id;
 		ns = tag__namespace(pos);
-		list_for_each_entry(annot, &ns->annots, node) {
+		list_for_each_entry(annot, &ns->tag.annots, node) {
+			if (annot->kind != BTF_DECL_TAG)
+				continue;
+
 			tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_type_id, annot->component_idx);
 			if (tag_type_id < 0) {
 				fprintf(stderr, "error: failed to encode tag '%s' to %s '%s' with component_idx %d\n",
diff --git a/dwarf_loader.c b/dwarf_loader.c
index 4efa4e1..17a2773 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -112,6 +112,17 @@ static dwarf_off_ref dwarf_tag__spec(struct dwarf_tag *dtag)
 	return *(dwarf_off_ref *)(dtag + 1);
 }
 
+#define cu__tag_not_handled(die) __cu__tag_not_handled(die, __FUNCTION__)
+
+static void __cu__tag_not_handled(Dwarf_Die *die, const char *fn)
+{
+	uint32_t tag = dwarf_tag(die);
+
+	fprintf(stderr, "%s: DW_TAG_%s (%#x) @ <%#llx> not handled!\n",
+		fn, dwarf_tag_name(tag), tag,
+		(unsigned long long)dwarf_dieoffset(die));
+}
+
 static void dwarf_tag__set_spec(struct dwarf_tag *dtag, dwarf_off_ref spec)
 {
 	*(dwarf_off_ref *)(dtag + 1) = spec;
@@ -519,6 +530,7 @@ static void tag__init(struct tag *tag, struct cu *cu, Dwarf_Die *die)
 	}
 
 	INIT_LIST_HEAD(&tag->node);
+	INIT_LIST_HEAD(&tag->annots);
 }
 
 static struct tag *tag__new(Dwarf_Die *die, struct cu *cu)
@@ -608,7 +620,6 @@ static void namespace__init(struct namespace *namespace, Dwarf_Die *die,
 {
 	tag__init(&namespace->tag, cu, die);
 	INIT_LIST_HEAD(&namespace->tags);
-	INIT_LIST_HEAD(&namespace->annots);
 	namespace->name  = attr_string(die, DW_AT_name, conf);
 	namespace->nr_tags = 0;
 	namespace->shared_tags = 0;
@@ -876,8 +887,40 @@ static int tag__recode_dwarf_bitfield(struct tag *tag, struct cu *cu, uint16_t b
 	return -ENOMEM;
 }
 
-static int add_llvm_annotation(Dwarf_Die *die, int component_idx, struct conf_load *conf,
-			       struct list_head *head)
+static struct llvm_annotation *die__create_new_llvm_annotation(Dwarf_Die *die,
+							       struct cu *cu,
+							       struct conf_load *conf)
+{
+	struct llvm_annotation *tag;
+
+	tag = tag__alloc_with_spec(cu, sizeof(struct llvm_annotation));
+	if (tag == NULL)
+		return NULL;
+
+	tag__init(&tag->tag, cu, die);
+	return tag;
+}
+
+/** Allocate small_id for specified @tag */
+static int cu__assign_tag_id(struct cu *cu, struct tag *tag)
+{
+	struct dwarf_tag *dtag = tag->priv;
+	uint32_t id;
+
+	if (cu__table_add_tag(cu, tag, &id) < 0)
+		return -ENOMEM;
+
+	dtag->small_id = id;
+	cu__hash(cu, tag);
+
+	return 0;
+}
+
+static int add_btf_decl_tag(Dwarf_Die *die,
+			    struct cu *cu,
+			    int component_idx,
+			    struct conf_load *conf,
+			    struct list_head *head)
 {
 	struct llvm_annotation *annot;
 	const char *name;
@@ -890,17 +933,57 @@ static int add_llvm_annotation(Dwarf_Die *die, int component_idx, struct conf_lo
 	if (strcmp(name, "btf_decl_tag") != 0)
 		return 0;
 
-	annot = zalloc(sizeof(*annot));
+	annot = die__create_new_llvm_annotation(die, cu, conf);
 	if (!annot)
 		return -ENOMEM;
 
+	/* Don't assign id for btf_decl_tag */
+
+	annot->kind = BTF_DECL_TAG;
 	annot->value = attr_string(die, DW_AT_const_value, conf);
 	annot->component_idx = component_idx;
 	list_add_tail(&annot->node, head);
 	return 0;
 }
 
-static int add_child_llvm_annotations(Dwarf_Die *die, int component_idx,
+static int add_btf_type_tag(Dwarf_Die *die,
+			    struct cu *cu,
+			    struct conf_load *conf,
+			    struct list_head *head)
+{
+	struct llvm_annotation *annot;
+	const char *name;
+
+	if (conf->skip_encoding_btf_type_tag)
+		return 0;
+
+	name = attr_string(die, DW_AT_name, conf);
+
+	if (strcmp(name, "btf_type_tag") != 0)
+		return 0;
+
+	/* Create a btf_type_tag type for this annotation. */
+	annot = die__create_new_llvm_annotation(die, cu, conf);
+	if (annot == NULL)
+		return -ENOMEM;
+
+	cu__assign_tag_id(cu, &annot->tag);
+
+	annot->kind = BTF_TYPE_TAG_POINTEE;
+	annot->value = attr_string(die, DW_AT_const_value, conf);
+	annot->component_idx = -1;
+
+	/* For a list of DW_TAG_LLVM_annotation like tag1 -> tag2 -> tag3,
+	 * the tag->tags contains tag3 -> tag2 -> tag1.
+	 */
+	list_add(&annot->node, head);
+
+	return 0;
+}
+
+static int add_child_llvm_annotations(Dwarf_Die *die,
+				      struct cu *cu,
+				      int component_idx,
 				      struct conf_load *conf, struct list_head *head)
 {
 	Dwarf_Die child;
@@ -912,9 +995,14 @@ static int add_child_llvm_annotations(Dwarf_Die *die, int component_idx,
 	die = &child;
 	do {
 		if (dwarf_tag(die) == DW_TAG_LLVM_annotation) {
-			ret = add_llvm_annotation(die, component_idx, conf, head);
+			ret = add_btf_decl_tag(die, cu, component_idx, conf, head);
 			if (ret)
 				return ret;
+			ret = add_btf_type_tag(die, cu, conf, head);
+			if (ret)
+				return ret;
+		} else {
+			cu__tag_not_handled(die);
 		}
 	} while (dwarf_siblingof(die, die) == 0);
 
@@ -1340,19 +1428,8 @@ static uint64_t attr_upper_bound(Dwarf_Die *die)
 	return 0;
 }
 
-static void __cu__tag_not_handled(Dwarf_Die *die, const char *fn)
-{
-	uint32_t tag = dwarf_tag(die);
-
-	fprintf(stderr, "%s: DW_TAG_%s (%#x) @ <%#llx> not handled!\n",
-		fn, dwarf_tag_name(tag), tag,
-		(unsigned long long)dwarf_dieoffset(die));
-}
-
 static struct tag unsupported_tag;
 
-#define cu__tag_not_handled(die) __cu__tag_not_handled(die, __FUNCTION__)
-
 static struct tag *__die__process_tag(Dwarf_Die *die, struct cu *cu,
 				      int toplevel, const char *fn, struct conf_load *conf);
 
@@ -1372,89 +1449,17 @@ static struct tag *die__create_new_tag(Dwarf_Die *die, struct cu *cu)
 	return tag;
 }
 
-static struct btf_type_tag_ptr_type *die__create_new_btf_type_tag_ptr_type(Dwarf_Die *die, struct cu *cu)
+static struct tag *die__create_new_annotated_tag(Dwarf_Die *die, struct cu *cu,
+						 struct conf_load *conf)
 {
-	struct btf_type_tag_ptr_type *tag;
-
-	tag  = tag__alloc_with_spec(cu, sizeof(struct btf_type_tag_ptr_type));
-	if (tag == NULL)
-		return NULL;
-
-	tag__init(&tag->tag, cu, die);
-	tag->tag.has_btf_type_tag = true;
-	INIT_LIST_HEAD(&tag->tags);
-	return tag;
-}
-
-static struct btf_type_tag_type *die__create_new_btf_type_tag_type(Dwarf_Die *die, struct cu *cu,
-								   struct conf_load *conf)
-{
-	struct btf_type_tag_type *tag;
+	struct tag *tag = tag__new(die, cu);
 
-	tag  = tag__alloc_with_spec(cu, sizeof(struct btf_type_tag_type));
-	if (tag == NULL)
+	if (add_child_llvm_annotations(die, cu, -1, conf, &tag->annots))
 		return NULL;
 
-	tag__init(&tag->tag, cu, die);
-	tag->value = attr_string(die, DW_AT_const_value, conf);
 	return tag;
 }
 
-static struct tag *die__create_new_pointer_tag(Dwarf_Die *die, struct cu *cu,
-					       struct conf_load *conf)
-{
-	struct btf_type_tag_ptr_type *tag = NULL;
-	struct btf_type_tag_type *annot;
-	Dwarf_Die *cdie, child;
-	const char *name;
-	uint32_t id;
-
-	/* If no child tags or skipping btf_type_tag encoding, just create a new tag
-	 * and return
-	 */
-	if (!dwarf_haschildren(die) || dwarf_child(die, &child) != 0 ||
-	    conf->skip_encoding_btf_type_tag)
-		return tag__new(die, cu);
-
-	/* Otherwise, check DW_TAG_LLVM_annotation child tags */
-	cdie = &child;
-	do {
-		if (dwarf_tag(cdie) != DW_TAG_LLVM_annotation)
-			continue;
-
-		/* Only check btf_type_tag annotations */
-		name = attr_string(cdie, DW_AT_name, conf);
-		if (strcmp(name, "btf_type_tag") != 0)
-			continue;
-
-		if (tag == NULL) {
-			/* Create a btf_type_tag_ptr type. */
-			tag = die__create_new_btf_type_tag_ptr_type(die, cu);
-			if (!tag)
-				return NULL;
-		}
-
-		/* Create a btf_type_tag type for this annotation. */
-		annot = die__create_new_btf_type_tag_type(cdie, cu, conf);
-		if (annot == NULL)
-			return NULL;
-
-		if (cu__table_add_tag(cu, &annot->tag, &id) < 0)
-			return NULL;
-
-		struct dwarf_tag *dtag = annot->tag.priv;
-		dtag->small_id = id;
-		cu__hash(cu, &annot->tag);
-
-		/* For a list of DW_TAG_LLVM_annotation like tag1 -> tag2 -> tag3,
-		 * the tag->tags contains tag3 -> tag2 -> tag1.
-		 */
-		list_add(&annot->node, &tag->tags);
-	} while (dwarf_siblingof(cdie, cdie) == 0);
-
-	return tag ? &tag->tag : tag__new(die, cu);
-}
-
 static struct tag *die__create_new_ptr_to_member_type(Dwarf_Die *die,
 						      struct cu *cu)
 {
@@ -1541,7 +1546,7 @@ static struct tag *die__create_new_typedef(Dwarf_Die *die, struct cu *cu, struct
 	if (tdef == NULL)
 		return NULL;
 
-	if (add_child_llvm_annotations(die, -1, conf, &tdef->namespace.annots))
+	if (add_child_llvm_annotations(die, cu, -1, conf, &tdef->namespace.tag.annots))
 		return NULL;
 
 	return &tdef->namespace.tag;
@@ -1610,7 +1615,8 @@ static struct tag *die__create_new_parameter(Dwarf_Die *die,
 	if (ftype != NULL) {
 		ftype__add_parameter(ftype, parm);
 		if (param_idx >= 0) {
-			if (add_child_llvm_annotations(die, param_idx, conf, &(tag__function(&ftype->tag)->annots)))
+			if (add_child_llvm_annotations(die, cu, param_idx, conf,
+						       &ftype->tag.annots))
 				return NULL;
 		}
 	} else {
@@ -1651,7 +1657,7 @@ static struct tag *die__create_new_variable(Dwarf_Die *die, struct cu *cu, struc
 {
 	struct variable *var = variable__new(die, cu, conf);
 
-	if (var == NULL || add_child_llvm_annotations(die, -1, conf, &var->annots))
+	if (var == NULL || add_child_llvm_annotations(die, cu, -1, conf, &var->annots))
 		return NULL;
 
 	return &var->ip.tag;
@@ -1806,13 +1812,16 @@ static int die__process_class(Dwarf_Die *die, struct type *class,
 
 			type__add_member(class, member);
 			cu__hash(cu, &member->tag);
-			if (add_child_llvm_annotations(die, member_idx, conf, &class->namespace.annots))
+			if (add_child_llvm_annotations(die, cu, member_idx, conf,
+						       &class->namespace.tag.annots))
 				return -ENOMEM;
 			member_idx++;
 		}
 			continue;
 		case DW_TAG_LLVM_annotation:
-			if (add_llvm_annotation(die, -1, conf, &class->namespace.annots))
+			if (add_btf_decl_tag(die, cu, -1, conf, &class->namespace.tag.annots))
+				return -ENOMEM;
+			if (add_btf_type_tag(die, cu, conf, &class->namespace.tag.annots))
 				return -ENOMEM;
 			continue;
 		default: {
@@ -2089,7 +2098,8 @@ static int die__process_function(Dwarf_Die *die, struct ftype *ftype,
 				goto out_enomem;
 			continue;
 		case DW_TAG_LLVM_annotation:
-			if (add_llvm_annotation(die, -1, conf, &(tag__function(&ftype->tag)->annots)))
+			if (add_btf_decl_tag(die, cu, -1, conf,
+					     &(tag__function(&ftype->tag)->annots)))
 				goto out_enomem;
 			continue;
 		default:
@@ -2165,7 +2175,7 @@ static struct tag *__die__process_tag(Dwarf_Die *die, struct cu *cu,
 	case DW_TAG_unspecified_type:
 		tag = die__create_new_tag(die, cu);		break;
 	case DW_TAG_pointer_type:
-		tag = die__create_new_pointer_tag(die, cu, conf);	break;
+		tag = die__create_new_annotated_tag(die, cu, conf); break;
 	case DW_TAG_ptr_to_member_type:
 		tag = die__create_new_ptr_to_member_type(die, cu); break;
 	case DW_TAG_enumeration_type:
@@ -2493,10 +2503,10 @@ static void lexblock__recode_dwarf_types(struct lexblock *tag, struct cu *cu)
 	}
 }
 
-static void dwarf_cu__recode_btf_type_tag_ptr(struct btf_type_tag_ptr_type *tag,
+static void dwarf_cu__recode_btf_type_tag_ptr(struct tag *tag,
 					      uint32_t pointee_type)
 {
-	struct btf_type_tag_type *annot;
+	struct llvm_annotation *annot;
 	struct dwarf_tag *annot_dtag;
 	struct tag *prev_tag;
 
@@ -2523,8 +2533,8 @@ static void dwarf_cu__recode_btf_type_tag_ptr(struct btf_type_tag_ptr_type *tag,
 	 *   int tag1 tag2 tag3 *p;
 	 * and this matches the user/kernel code.
 	 */
-	prev_tag = &tag->tag;
-	list_for_each_entry(annot, &tag->tags, node) {
+	prev_tag = tag;
+	list_for_each_entry(annot, &tag->annots, node) {
 		annot_dtag = annot->tag.priv;
 		prev_tag->type = annot_dtag->small_id;
 		prev_tag = &annot->tag;
@@ -2636,15 +2646,17 @@ static int tag__recode_dwarf_type(struct tag *tag, struct cu *cu)
 					var->spec = tag__variable(dtype->tag);
 			}
 		}
+		break;
 	}
-
+	case DW_TAG_LLVM_annotation:
+		return 0;
 	}
 
 	if (dtag->type.off == 0) {
-		if (tag->tag != DW_TAG_pointer_type || !tag->has_btf_type_tag)
+		if (tag->tag != DW_TAG_pointer_type)
 			tag->type = 0; /* void */
 		else
-			dwarf_cu__recode_btf_type_tag_ptr(tag__btf_type_tag_ptr(tag), 0);
+			dwarf_cu__recode_btf_type_tag_ptr(tag, 0);
 		return 0;
 	}
 
@@ -2656,10 +2668,10 @@ check_type:
 		return 0;
 	}
 out:
-	if (tag->tag != DW_TAG_pointer_type || !tag->has_btf_type_tag)
+	if (tag->tag != DW_TAG_pointer_type)
 		tag->type = dtype->small_id;
 	else
-		dwarf_cu__recode_btf_type_tag_ptr(tag__btf_type_tag_ptr(tag), dtype->small_id);
+		dwarf_cu__recode_btf_type_tag_ptr(tag, dtype->small_id);
 
 	return 0;
 }
diff --git a/dwarves.h b/dwarves.h
index 7a319d1..0b0b0cc 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -419,8 +419,8 @@ int cu__for_all_tags(struct cu *cu,
 		     void *cookie);
 
 /** struct tag - basic representation of a debug info element
- * @priv - extra data, for instance, DWARF offset, id, decl_{file,line}
- * @top_level -
+ * @priv   - extra data, for instance, DWARF offset, id, decl_{file,line}
+ * @annots - list of btf_type_tag and btf_decl_tag annotations.
  */
 struct tag {
 	struct list_head node;
@@ -428,8 +428,8 @@ struct tag {
 	uint16_t	 tag;
 	bool		 visited;
 	bool		 top_level;
-	bool		 has_btf_type_tag;
 	uint16_t	 recursivity_level;
+	struct list_head annots;
 	void		 *priv;
 };
 
@@ -625,43 +625,31 @@ static inline struct ptr_to_member_type *
 	return (struct ptr_to_member_type *)tag;
 }
 
-struct llvm_annotation {
-	const char		*value;
-	int16_t			component_idx;
-	struct list_head	node;
+enum annotation_kind {
+	BTF_DECL_TAG,
+	/* "btf_type_tag" in DWARF, attached to a pointer, applies to pointee type */
+	BTF_TYPE_TAG_POINTEE,
 };
 
-/** struct btf_type_tag_type - representing a btf_type_tag annotation
+/** struct llvm_annotation - representing objects with DW_TAG_LLVM_annotation tag
  *
- * @tag   - DW_TAG_LLVM_annotation tag
- * @value - btf_type_tag value string
- * @node  - list_head node
+ * @tag           - DW_TAG_LLVM_annotation tag
+ * @kind          - annotation kind
+ * @value         - value string, valid for both "btf_decl_tag" and "btf_type_tag"
+ * @component_idx - component index, valid only for "btf_decl_tag"
+ * @node          - list_head node
  */
-struct btf_type_tag_type {
+struct llvm_annotation {
 	struct tag		tag;
+	enum annotation_kind	kind;
 	const char		*value;
+	int16_t			component_idx;
 	struct list_head	node;
 };
 
-/** The struct btf_type_tag_ptr_type - type containing both pointer type and
- *  its btf_type_tag annotations
- *
- * @tag  - pointer type tag
- * @tags - btf_type_tag annotations for the pointer type
- */
-struct btf_type_tag_ptr_type {
-	struct tag		tag;
-	struct list_head 	tags;
-};
-
-static inline struct btf_type_tag_ptr_type *tag__btf_type_tag_ptr(struct tag *tag)
+static inline struct llvm_annotation *tag__llvm_annotation(struct tag *tag)
 {
-	return (struct btf_type_tag_ptr_type *)tag;
-}
-
-static inline struct btf_type_tag_type *tag__btf_type_tag(struct tag *tag)
-{
-	return (struct btf_type_tag_type *)tag;
+	return (struct llvm_annotation *)tag;
 }
 
 /** struct namespace - base class for enums, structs, unions, typedefs, etc
@@ -675,7 +663,6 @@ struct namespace {
 	uint16_t	 nr_tags;
 	uint8_t		 shared_tags;
 	struct list_head tags;
-	struct list_head annots;
 };
 
 static inline struct namespace *tag__namespace(const struct tag *tag)
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH dwarves v2 4/5] dwarf_loader: Track unspecified types in a separate list
  2023-03-14 23:04 [PATCH dwarves v2 0/5] Support for new btf_type_tag encoding Eduard Zingerman
                   ` (2 preceding siblings ...)
  2023-03-14 23:04 ` [PATCH dwarves v2 3/5] dwarf_loader: Consolidate llvm_annotation and btf_type_tag_type Eduard Zingerman
@ 2023-03-14 23:04 ` Eduard Zingerman
  2023-03-14 23:04 ` [PATCH dwarves v2 5/5] dwarf_loader: Support for btf:type_tag Eduard Zingerman
  4 siblings, 0 replies; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-14 23:04 UTC (permalink / raw)
  To: dwarves, arnaldo.melo
  Cc: bpf, kernel-team, ast, daniel, andrii, yhs, jose.marchesi,
	david.faust, alan.maguire, Eduard Zingerman

In recent discussion in BPF mailing list ([1]) participants agreed to
add a new DWARF representation for "btf_type_tag" annotations.
The agreed representation of void pointers uses unspecified types.
For example, consider the following C code:

    struct alpha {
      void __attribute__((btf_type_tag("__alpha_a"))) *a;
    } g;

And corresponding DWARF:

0x29:   DW_TAG_structure_type
          DW_AT_name      ("alpha")

0x2e:     DW_TAG_member
            DW_AT_name    ("a")
            DW_AT_type    (0x38 "void *")

0x38:   DW_TAG_pointer_type
          DW_AT_type      (0x41 "void")

0x41:   DW_TAG_unspecified_type
          DW_AT_name      ("void")

0x43:     DW_TAG_LLVM_annotation
            DW_AT_name    ("btf:type_tag")
            DW_AT_const_value     ("__alpha_a")

This is a preparatory patch for new type tags representation support,
specifically it adds `struct unspecified_type` and a new `cu` field
`struct cu::unspecified_types`. These would be used in a subsequent
patch to recode type tags attached to DW_TAG_unspecified_type
as in the example above.

[1] Mailing list discussion regarding `btf:type_tag`
    Various approaches are discussed, Solution #2 is accepted
    https://lore.kernel.org/bpf/87r0w9jjoq.fsf@oracle.com/

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 dwarf_loader.c | 36 ++++++++++++++++++++++++++++++++++--
 dwarves.c      |  1 +
 dwarves.h      | 17 +++++++++++++++++
 3 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 17a2773..218806b 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -176,14 +176,19 @@ static struct dwarf_cu *dwarf_cu__new(struct cu *cu)
 	return dwarf_cu;
 }
 
+static void unspecified_type__delete(struct cu *cu, struct unspecified_type *utype);
+
 static void dwarf_cu__delete(struct cu *cu)
 {
 	if (cu == NULL || cu->priv == NULL)
 		return;
 
 	struct dwarf_cu *dcu = cu->priv;
+	struct list_head *pos, *n;
 
 	// dcu->hash_tags & dcu->hash_types are on cu->obstack
+	list_for_each_safe(pos, n, &cu->unspecified_types)
+		unspecified_type__delete(cu, container_of(pos, struct unspecified_type, node));
 	cu__free(cu, dcu);
 	cu->priv = NULL;
 }
@@ -1449,6 +1454,33 @@ static struct tag *die__create_new_tag(Dwarf_Die *die, struct cu *cu)
 	return tag;
 }
 
+static struct tag *die__create_new_unspecified_type(Dwarf_Die *die, struct cu *cu,
+						    struct conf_load *conf)
+{
+	struct unspecified_type *tag;
+
+	tag = tag__alloc_with_spec(cu, sizeof(struct unspecified_type));
+	if (tag == NULL)
+		return NULL;
+
+	tag__init(&tag->tag, cu, die);
+	INIT_LIST_HEAD(&tag->node);
+
+	tag->name = attr_string(die, DW_AT_name, conf);
+
+	list_add(&tag->node, &cu->unspecified_types);
+
+	return &tag->tag;
+}
+
+static void unspecified_type__delete(struct cu *cu, struct unspecified_type *utype)
+{
+	struct dwarf_tag *dtag = utype->tag.priv;
+
+	cu__free(cu, dtag);
+	cu__free(cu, utype);
+}
+
 static struct tag *die__create_new_annotated_tag(Dwarf_Die *die, struct cu *cu,
 						 struct conf_load *conf)
 {
@@ -2172,10 +2204,10 @@ static struct tag *__die__process_tag(Dwarf_Die *die, struct cu *cu,
 	case DW_TAG_volatile_type:
 	case DW_TAG_atomic_type:
 		tag = die__create_new_tag(die, cu);		break;
-	case DW_TAG_unspecified_type:
-		tag = die__create_new_tag(die, cu);		break;
 	case DW_TAG_pointer_type:
 		tag = die__create_new_annotated_tag(die, cu, conf); break;
+	case DW_TAG_unspecified_type:
+		tag = die__create_new_unspecified_type(die, cu, conf); break;
 	case DW_TAG_ptr_to_member_type:
 		tag = die__create_new_ptr_to_member_type(die, cu); break;
 	case DW_TAG_enumeration_type:
diff --git a/dwarves.c b/dwarves.c
index b43031c..7e66a98 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -681,6 +681,7 @@ struct cu *cu__new(const char *name, uint8_t addr_size,
 		cu->dfops	= NULL;
 		INIT_LIST_HEAD(&cu->tags);
 		INIT_LIST_HEAD(&cu->tool_list);
+		INIT_LIST_HEAD(&cu->unspecified_types);
 
 		cu->addr_size = addr_size;
 		cu->extra_dbg_info = 0;
diff --git a/dwarves.h b/dwarves.h
index 0b0b0cc..cbd2913 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -242,6 +242,7 @@ struct cu {
 	struct list_head node;
 	struct list_head tags;
 	struct list_head tool_list;	/* To be used by tools such as ctracer */
+	struct list_head unspecified_types;
 	struct ptr_table types_table;
 	struct ptr_table functions_table;
 	struct ptr_table tags_table;
@@ -652,6 +653,22 @@ static inline struct llvm_annotation *tag__llvm_annotation(struct tag *tag)
 	return (struct llvm_annotation *)tag;
 }
 
+/** struct unspecified_type - representation of DW_TAG_unspecified_type.
+ *
+ *  @name   - DW_AT_name associated with this tag
+ *  @node   - a node for cu::unspecified_types list
+ */
+struct unspecified_type {
+	struct tag		tag;
+	const char		*name;
+	struct list_head	node;
+};
+
+static inline struct unspecified_type *tag__unspecified_type(struct tag *tag)
+{
+	return (struct unspecified_type *)tag;
+}
+
 /** struct namespace - base class for enums, structs, unions, typedefs, etc
  *
  * @tags - class_member, enumerators, etc
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH dwarves v2 5/5] dwarf_loader: Support for btf:type_tag
  2023-03-14 23:04 [PATCH dwarves v2 0/5] Support for new btf_type_tag encoding Eduard Zingerman
                   ` (3 preceding siblings ...)
  2023-03-14 23:04 ` [PATCH dwarves v2 4/5] dwarf_loader: Track unspecified types in a separate list Eduard Zingerman
@ 2023-03-14 23:04 ` Eduard Zingerman
  4 siblings, 0 replies; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-14 23:04 UTC (permalink / raw)
  To: dwarves, arnaldo.melo
  Cc: bpf, kernel-team, ast, daniel, andrii, yhs, jose.marchesi,
	david.faust, alan.maguire, Eduard Zingerman

"btf:type_tag" is an DW_TAG_LLVM_annotation object that encodes
btf_type_tag attributes in DWARF. Contrary to existing "btf_type_tag"
it allows to associate such attributes with non-pointer types.
When "btf:type_tag" is attached to a type it applies to this type.

For example the following C code:

    struct echo {
      int __attribute__((btf_type_tag("__c"))) c;
    }

Produces the following DWARF:

0x29:   DW_TAG_structure_type
          DW_AT_name      ("echo")

0x40:     DW_TAG_member
            DW_AT_name    ("c")
            DW_AT_type    (0x8c "int")

0x8c:   DW_TAG_base_type
          DW_AT_name      ("int")
          DW_AT_encoding  (DW_ATE_signed)
          DW_AT_byte_size (0x04)

0x90:     DW_TAG_LLVM_annotation
            DW_AT_name    ("btf:type_tag")
            DW_AT_const_value     ("__c")

Meaning that type 0x8c is an `int` with type tag `__c`.
Corresponding BTF looks as follows:

[1] STRUCT 'echo'
        ...
        'c' type_id=8 bits_offset=128
[4] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
[8] TYPE_TAG '__c' type_id=4

This commit adds support for DW_TAG_LLVM_annotation "btf:type_tag"
attached to the following entities:
- base types;
- arrays;
- pointers;
- structs
- unions;
- enums;
- typedefs.

To allow backwards compatibility and void additional invocation
options, implementation acts in a following way:
- both `btf_type_tag` and `btf:type_tag` could be present in the
  debug info;
- if `btf:type_tag` are present in the debug info, `btf_type_tag`
  annotations are ignored.

Modifications done by this commit:
- DWARF load phase is updated:
  - `annots` fields are filled for the above mentioned types;
  - `cu` instance is updated to reflect which version of type tags is
    used in the debug info;
- Recode phase is split in several sub-phases:
  - `cu__allocate_btf_type_tags()`
    `llvm_annotation` instances corresponding to preferred version of
    type tags are added to types table;
  - `tag__recode_dwarf_type()` (the original phase logic);
  - `update_btf_type_tag_refs()`
    Updates `->type` field of each tag if that type refers to a type
    with `btf:type_tag` annotation. The id of the type is replaced by
    id of the type tag.

See also:
[1] Mailing list discussion regarding `btf:type_tag`
    Various approaches are discussed, Solution #2 is accepted
    https://lore.kernel.org/bpf/87r0w9jjoq.fsf@oracle.com/

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 dwarf_loader.c | 530 ++++++++++++++++++++++++++++++++++++++++++++-----
 dwarves.h      |  10 +-
 2 files changed, 484 insertions(+), 56 deletions(-)

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 218806b..fe38c29 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -57,6 +57,36 @@
 #define EM_RISCV	243
 #endif
 
+/** struct btf_type_tag_mapping - information about btf type tags attached
+ *  to a particular host type. Each field is a small_id of the dwarf tag.
+ *  This information is used to replace references to host type with
+ *  references to first btf type tag during the recode phase.
+ *
+ *  @host_type_id - type annotated with btf_type_tag annotations
+ *  @first_tag_id - first btf type tag attached to host_type_id
+ *  @last_tag_id  - last btf type tag attached to host_type_id
+ *
+ */
+struct btf_type_tag_mapping {
+	uint32_t host_type_id;
+	uint32_t first_tag_id;
+	uint32_t last_tag_id;
+};
+
+/** struct recode_context - information local to recode phase,
+ *  currently only btf type tag mappings.
+ *
+ *  @mappings     - an array of host id to btf type tag mappings,
+ *                  dynamically enlarged when new mappings are added;
+ *  @nr_allocated - number of elements allocated for @mappings array;
+ *  @nr_entries   - index of the next free entry in the @mappings array.
+ */
+struct recode_context {
+	struct btf_type_tag_mapping *mappings;
+	uint32_t nr_allocated;
+	uint32_t nr_entries;
+};
+
 static pthread_mutex_t libdw__lock = PTHREAD_MUTEX_INITIALIZER;
 
 static uint32_t hashtags__bits = 12;
@@ -958,13 +988,16 @@ static int add_btf_type_tag(Dwarf_Die *die,
 {
 	struct llvm_annotation *annot;
 	const char *name;
+	bool v1, v2;
 
 	if (conf->skip_encoding_btf_type_tag)
 		return 0;
 
 	name = attr_string(die, DW_AT_name, conf);
+	v1 = strcmp(name, "btf_type_tag") == 0;
+	v2 = strcmp(name, "btf:type_tag") == 0;
 
-	if (strcmp(name, "btf_type_tag") != 0)
+	if (!v1 && !v2)
 		return 0;
 
 	/* Create a btf_type_tag type for this annotation. */
@@ -972,11 +1005,11 @@ static int add_btf_type_tag(Dwarf_Die *die,
 	if (annot == NULL)
 		return -ENOMEM;
 
-	cu__assign_tag_id(cu, &annot->tag);
-
-	annot->kind = BTF_TYPE_TAG_POINTEE;
+	annot->kind = v2 ? BTF_TYPE_TAG : BTF_TYPE_TAG_POINTEE;
 	annot->value = attr_string(die, DW_AT_const_value, conf);
 	annot->component_idx = -1;
+	if (v2)
+		cu->ignore_btf_type_tag_pointee = 1;
 
 	/* For a list of DW_TAG_LLVM_annotation like tag1 -> tag2 -> tag3,
 	 * the tag->tags contains tag3 -> tag2 -> tag1.
@@ -1467,6 +1500,8 @@ static struct tag *die__create_new_unspecified_type(Dwarf_Die *die, struct cu *c
 	INIT_LIST_HEAD(&tag->node);
 
 	tag->name = attr_string(die, DW_AT_name, conf);
+	if (add_child_llvm_annotations(die, cu, -1, conf, &tag->tag.annots))
+		return NULL;
 
 	list_add(&tag->node, &cu->unspecified_types);
 
@@ -1564,9 +1599,8 @@ static struct tag *die__create_new_base_type(Dwarf_Die *die, struct cu *cu, stru
 	if (base == NULL)
 		return NULL;
 
-	if (dwarf_haschildren(die))
-		fprintf(stderr, "%s: DW_TAG_base_type WITH children!\n",
-			__func__);
+	if (add_child_llvm_annotations(die, cu, -1, conf, &base->tag.annots))
+		return NULL;
 
 	return &base->tag;
 }
@@ -1584,7 +1618,7 @@ static struct tag *die__create_new_typedef(Dwarf_Die *die, struct cu *cu, struct
 	return &tdef->namespace.tag;
 }
 
-static struct tag *die__create_new_array(Dwarf_Die *die, struct cu *cu)
+static struct tag *die__create_new_array(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
 {
 	Dwarf_Die child;
 	/* "64 dimensions will be enough for everybody." acme, 2006 */
@@ -1600,17 +1634,25 @@ static struct tag *die__create_new_array(Dwarf_Die *die, struct cu *cu)
 
 	die = &child;
 	do {
-		if (dwarf_tag(die) == DW_TAG_subrange_type) {
+		switch (dwarf_tag(die)) {
+		case DW_TAG_subrange_type:
 			nr_entries[array->dimensions++] = attr_upper_bound(die);
 			if (array->dimensions == max_dimensions) {
 				fprintf(stderr, "%s: only %u dimensions are "
 						"supported!\n",
 					__FUNCTION__, max_dimensions);
-				break;
+				goto _break;
 			}
-		} else
+			break;
+		case DW_TAG_LLVM_annotation:
+			if (add_btf_type_tag(die, cu, conf, &array->tag.annots))
+				goto out_free;
+			break;
+		default:
 			cu__tag_not_handled(die);
+		}
 	} while (dwarf_siblingof(die, die) == 0);
+_break:
 
 	array->nr_entries = memdup(nr_entries,
 				   array->dimensions * sizeof(uint32_t), cu);
@@ -1722,6 +1764,10 @@ static struct tag *die__create_new_subroutine_type(Dwarf_Die *die,
 		case DW_TAG_unspecified_parameters:
 			ftype->unspec_parms = 1;
 			continue;
+		case DW_TAG_LLVM_annotation:
+			if (add_btf_type_tag(die, cu, conf, &ftype->tag.annots))
+				goto out_delete;
+			continue;
 		default:
 			tag = die__process_tag(die, cu, 0, conf);
 			if (tag == NULL)
@@ -1778,17 +1824,25 @@ static struct tag *die__create_new_enumeration(Dwarf_Die *die, struct cu *cu, st
 
 	die = &child;
 	do {
-		struct enumerator *enumerator;
+		switch (dwarf_tag(die)) {
+		case DW_TAG_enumerator: {
+			struct enumerator *enumerator;
+
+			enumerator = enumerator__new(die, cu, conf);
+			if (enumerator == NULL)
+				goto out_delete;
 
-		if (dwarf_tag(die) != DW_TAG_enumerator) {
+			enumeration__add(enumeration, enumerator);
+			break;
+		}
+		case DW_TAG_LLVM_annotation:
+			if (add_btf_type_tag(die, cu, conf,
+					     &enumeration->namespace.tag.annots))
+				goto out_delete;
+			break;
+		default:
 			cu__tag_not_handled(die);
-			continue;
 		}
-		enumerator = enumerator__new(die, cu, conf);
-		if (enumerator == NULL)
-			goto out_delete;
-
-		enumeration__add(enumeration, enumerator);
 	} while (dwarf_siblingof(die, die) == 0);
 out:
 	return &enumeration->namespace.tag;
@@ -2191,19 +2245,19 @@ static struct tag *__die__process_tag(Dwarf_Die *die, struct cu *cu,
 	case DW_TAG_imported_unit:
 		return NULL; // We don't support imported units yet, so to avoid segfaults
 	case DW_TAG_array_type:
-		tag = die__create_new_array(die, cu);		break;
+		tag = die__create_new_array(die, cu, conf);	break;
 	case DW_TAG_string_type: // FORTRAN stuff, looks like an array
 		tag = die__create_new_string_type(die, cu);	break;
 	case DW_TAG_base_type:
 		tag = die__create_new_base_type(die, cu, conf);	break;
-	case DW_TAG_const_type:
 	case DW_TAG_imported_declaration:
 	case DW_TAG_imported_module:
 	case DW_TAG_reference_type:
-	case DW_TAG_restrict_type:
-	case DW_TAG_volatile_type:
 	case DW_TAG_atomic_type:
 		tag = die__create_new_tag(die, cu);		break;
+	case DW_TAG_const_type:
+	case DW_TAG_restrict_type:
+	case DW_TAG_volatile_type:
 	case DW_TAG_pointer_type:
 		tag = die__create_new_annotated_tag(die, cu, conf); break;
 	case DW_TAG_unspecified_type:
@@ -2294,20 +2348,99 @@ static int die__process_unit(Dwarf_Die *die, struct cu *cu, struct conf_load *co
 	return 0;
 }
 
-static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu);
+/** Add @tuple to @ctx->mappings array, extend it if necessary. */
+static int push_btf_type_tag_mapping(struct btf_type_tag_mapping *tuple,
+				     struct recode_context *ctx)
+{
+	if (ctx->nr_allocated == ctx->nr_entries) {
+		uint32_t new_nr = ctx->nr_allocated * 2;
+		void *new_array = realloc(ctx->mappings, new_nr * sizeof(ctx->mappings[0]));
+		if (!new_array)
+			return -ENOMEM;
+		ctx->mappings = new_array;
+		ctx->nr_allocated = new_nr;
+	}
+
+	ctx->mappings[ctx->nr_entries++] = *tuple;
 
-static int namespace__recode_dwarf_types(struct tag *tag, struct cu *cu)
+	return 0;
+}
+
+/** Connect `type` fields of btf:type_tag annotations attached to a
+ *  host type. Collect information about first and last tag.
+ *  `type` field are connected as below:
+ *
+ *    tag1.type -> tag2.type -> host_type
+ *
+ *  @tags      - list of llvm_annotation objects;
+ *  @host_type - small_id of the type with attached annotations.
+ */
+static void __recode_btf_type_tags(struct list_head *tags,
+				   uint32_t host_type,
+				   struct btf_type_tag_mapping *mapping)
+{
+	struct llvm_annotation *annot;
+	struct dwarf_tag *annot_dtag;
+	struct tag *prev_tag = NULL;
+	uint32_t first_tag_id = 0;
+
+	list_for_each_entry(annot, tags, node) {
+		if (annot->kind != BTF_TYPE_TAG)
+			continue;
+		annot_dtag = annot->tag.priv;
+		if (prev_tag)
+			prev_tag->type = annot_dtag->small_id;
+		if (!first_tag_id)
+			first_tag_id = annot_dtag->small_id;
+		prev_tag = &annot->tag;
+	}
+
+	mapping->host_type_id = host_type;
+	if (prev_tag) {
+		prev_tag->type = host_type;
+		mapping->first_tag_id = first_tag_id;
+		mapping->last_tag_id = annot_dtag->small_id;
+	} else {
+		mapping->first_tag_id = 0;
+		mapping->last_tag_id = 0;
+	}
+}
+
+static int recode_btf_type_tags(struct list_head *tags,
+				uint32_t host_type,
+				struct recode_context *ctx)
+{
+	struct btf_type_tag_mapping mapping;
+
+	if (list_empty(tags))
+		return 0;
+
+	__recode_btf_type_tags(tags, host_type, &mapping);
+	if (!mapping.first_tag_id)
+		return 0;
+
+	return push_btf_type_tag_mapping(&mapping, ctx);
+}
+
+static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu,
+				      struct recode_context *ctx);
+
+static int namespace__recode_dwarf_types(struct tag *tag, struct cu *cu,
+					 struct recode_context *ctx)
 {
 	struct tag *pos;
 	struct dwarf_cu *dcu = cu->priv;
+	struct dwarf_tag *dtag = tag->priv;
 	struct namespace *ns = tag__namespace(tag);
 
+	recode_btf_type_tags(&tag->annots, dtag->small_id, ctx);
+
 	namespace__for_each_tag(ns, pos) {
 		struct dwarf_tag *dtype;
 		struct dwarf_tag *dpos = pos->priv;
 
 		if (tag__has_namespace(pos)) {
-			if (namespace__recode_dwarf_types(pos, cu))
+			if (namespace__recode_dwarf_types(pos, cu, ctx))
 				return -1;
 			continue;
 		}
@@ -2328,7 +2461,7 @@ static int namespace__recode_dwarf_types(struct tag *tag, struct cu *cu)
 			break;
 		case DW_TAG_subroutine_type:
 		case DW_TAG_subprogram:
-			ftype__recode_dwarf_types(pos, cu);
+			ftype__recode_dwarf_types(pos, cu, ctx);
 			break;
 		case DW_TAG_imported_module:
 			dtype = dwarf_cu__find_tag_by_ref(dcu, &dpos->type);
@@ -2393,7 +2526,8 @@ static void __tag__print_abstract_origin_not_found(struct tag *tag,
 #define tag__print_abstract_origin_not_found(tag ) \
 	__tag__print_abstract_origin_not_found(tag, __func__)
 
-static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
+static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu,
+				      struct recode_context *ctx)
 {
 	struct parameter *pos;
 	struct dwarf_cu *dcu = cu->priv;
@@ -2441,9 +2575,13 @@ static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
 		}
 		pos->tag.type = dtype->small_id;
 	}
+
+	struct dwarf_tag *dtag = tag->priv;
+	recode_btf_type_tags(&tag->annots, dtag->small_id, ctx);
 }
 
-static void lexblock__recode_dwarf_types(struct lexblock *tag, struct cu *cu)
+static void lexblock__recode_dwarf_types(struct lexblock *tag, struct cu *cu,
+					 struct recode_context *ctx)
 {
 	struct tag *pos;
 	struct dwarf_cu *dcu = cu->priv;
@@ -2454,7 +2592,7 @@ static void lexblock__recode_dwarf_types(struct lexblock *tag, struct cu *cu)
 
 		switch (pos->tag) {
 		case DW_TAG_lexical_block:
-			lexblock__recode_dwarf_types(tag__lexblock(pos), cu);
+			lexblock__recode_dwarf_types(tag__lexblock(pos), cu, ctx);
 			continue;
 		case DW_TAG_inlined_subroutine:
 			if (dpos->type.off != 0)
@@ -2468,7 +2606,7 @@ static void lexblock__recode_dwarf_types(struct lexblock *tag, struct cu *cu)
 					tag__print_abstract_origin_not_found(pos);
 				continue;
 			}
-			ftype__recode_dwarf_types(dtype->tag, cu);
+			ftype__recode_dwarf_types(dtype->tag, cu, ctx);
 			continue;
 
 		case DW_TAG_formal_parameter:
@@ -2535,8 +2673,144 @@ static void lexblock__recode_dwarf_types(struct lexblock *tag, struct cu *cu)
 	}
 }
 
+static int recode_context_init(struct recode_context *ctx)
+{
+	const int initial_nr = 16;
+
+	ctx->mappings = reallocarray(NULL, initial_nr, sizeof(ctx->mappings[0]));
+	if (!ctx->mappings)
+		return -ENOMEM;
+
+	ctx->nr_allocated = initial_nr;
+	ctx->nr_entries = 0;
+
+	return 0;
+}
+
+static void recode_context_free(struct recode_context *ctx)
+{
+	free(ctx->mappings);
+	ctx->nr_allocated = 0;
+	ctx->nr_entries = 0;
+}
+
+/** Compare two `btf_type_tag_mapping` objects using host_type_id as key. */
+static int compare_btf_type_tag_recode_mappings(const void *_a, const void *_b)
+{
+	const struct btf_type_tag_mapping *a = _a;
+	const struct btf_type_tag_mapping *b = _b;
+	long diff = (long)a->host_type_id - (long)b->host_type_id;
+
+	if (diff < 0)
+		return -1;
+	if (diff > 0)
+		return 1;
+	return 0;
+}
+
+/** Sort @ctx->mappings array by btf_type_tag_mapping::host_type_id,
+ *  function `lookup_btf_type_tag_by_host()` uses binary search to find
+ *  elements of this array.
+ */
+static void sort_btf_type_tags(struct recode_context *ctx)
+{
+	qsort(ctx->mappings, ctx->nr_entries, sizeof(ctx->mappings[0]),
+	      compare_btf_type_tag_recode_mappings);
+}
+
+static struct btf_type_tag_mapping *lookup_btf_type_tag_by_host(uint32_t host_type_id,
+								struct recode_context *ctx)
+{
+	struct btf_type_tag_mapping key = { .host_type_id = host_type_id };
+
+	return bsearch(&key, ctx->mappings, ctx->nr_entries, sizeof(key),
+		       compare_btf_type_tag_recode_mappings);
+}
+
+/** Update @tag->type fields by replacing types by ids of associated
+ *  btf:type_tag objects. @ctx->mappings should be sorted when this
+ *  function is called.
+ *
+ *  When "btf:type_tag" is attached to a type it applies to this type.
+ *  For example, the following dwarf:
+ *
+ *  0x00000040:     DW_TAG_member
+ *                    DW_AT_name    ("c")
+ *                    DW_AT_type    (0x0000008c "int")
+ *                    DW_AT_decl_file       ("/home/eddy/work/tmp/test.c")
+ *                    DW_AT_decl_line       (13)
+ *                    DW_AT_data_member_location    (0x10)
+ *
+ *  0x0000008c:   DW_TAG_base_type
+ *                  DW_AT_name      ("int")
+ *                  DW_AT_encoding  (DW_ATE_signed)
+ *                  DW_AT_byte_size (0x04)
+ *
+ *  0x00000090:     DW_TAG_LLVM_annotation
+ *                    DW_AT_name    ("btf:type_tag")
+ *                    DW_AT_const_value     ("__c")
+ *
+ *  Means that type 0x0000008c is an `int` with type tag `__c`.
+ *  Corresponding BTF looks as follows:
+ *
+ *  [1] STRUCT 'echo'
+ *          ...
+ *          'c' type_id=8 bits_offset=128
+ *  [4] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
+ *  [8] TYPE_TAG '__c' type_id=4
+ *
+ *  Before the call to `update_btf_type_tag_refs` for member 0x00000040
+ *  its `type` field points to 0x0000008c. `update_btf_type_tag_refs`
+ *  updates this link to point to `0x00000090` instead, thus obtaining the
+ *  desired BTF shape.
+ */
+static int update_btf_type_tag_refs(struct tag *tag,
+				    struct cu *cu,
+				    struct recode_context *ctx)
+{
+	struct btf_type_tag_mapping *tuple;
+	struct dwarf_tag *dtag = tag->priv;
+
+	/* Kernel does not support VAR entries with types of form
+	 * 'VAR -> TYPE_TAG -> something':
+	 * - in verifier.c:check_pseudo_btf_id() instruction auxiliary
+	 *   data is set to point to variable type w/o stripping modifiers;
+	 * - btf.c:btf_struct_access() -> btf.c:btf_struct_walk()
+	 *   does not skip modifiers prior to btf_type_is_struct() check.
+	 *
+	 * So, skip type tag recoding for variables.
+	 */
+	if (tag->tag == DW_TAG_variable)
+		return 0;
+
+	tuple = lookup_btf_type_tag_by_host(tag->type, ctx);
+	/* Avoid creation of circular references, last btf:type_tag
+	 * object points to the host type.
+	 */
+	if (tuple && tuple->last_tag_id != dtag->small_id)
+		tag->type = tuple->first_tag_id;
+
+	if (tag__is_type(tag)) {
+		struct type *type = tag__type(tag);
+		struct class_member *pos;
+
+		type__for_each_data_member(type, pos)
+			update_btf_type_tag_refs(&pos->tag, cu, ctx);
+	} else if (tag->tag == DW_TAG_subprogram ||
+		   tag->tag == DW_TAG_subroutine_type) {
+		struct ftype *ftype = tag__ftype(tag);
+		struct parameter *pos;
+
+		ftype__for_each_parameter(ftype, pos)
+			update_btf_type_tag_refs(&pos->tag, cu, ctx);
+	}
+
+	return 0;
+}
+
 static void dwarf_cu__recode_btf_type_tag_ptr(struct tag *tag,
-					      uint32_t pointee_type)
+					      uint32_t pointee_type,
+					      struct cu *cu)
 {
 	struct llvm_annotation *annot;
 	struct dwarf_tag *annot_dtag;
@@ -2566,15 +2840,19 @@ static void dwarf_cu__recode_btf_type_tag_ptr(struct tag *tag,
 	 * and this matches the user/kernel code.
 	 */
 	prev_tag = tag;
-	list_for_each_entry(annot, &tag->annots, node) {
-		annot_dtag = annot->tag.priv;
-		prev_tag->type = annot_dtag->small_id;
-		prev_tag = &annot->tag;
+	if (!cu->ignore_btf_type_tag_pointee) {
+		list_for_each_entry(annot, &tag->annots, node) {
+			if (annot->kind != BTF_TYPE_TAG_POINTEE)
+				continue;
+			annot_dtag = annot->tag.priv;
+			prev_tag->type = annot_dtag->small_id;
+			prev_tag = &annot->tag;
+		}
 	}
 	prev_tag->type = pointee_type;
 }
 
-static int tag__recode_dwarf_type(struct tag *tag, struct cu *cu)
+static int tag__recode_dwarf_type(struct tag *tag, struct cu *cu, struct recode_context *ctx)
 {
 	struct dwarf_tag *dtag = tag->priv;
 	struct dwarf_tag *dtype;
@@ -2587,7 +2865,7 @@ static int tag__recode_dwarf_type(struct tag *tag, struct cu *cu)
 		type__recode_dwarf_specification(tag, cu);
 
 	if (tag__has_namespace(tag))
-		return namespace__recode_dwarf_types(tag, cu);
+		return namespace__recode_dwarf_types(tag, cu, ctx);
 
 	switch (tag->tag) {
 	case DW_TAG_subprogram: {
@@ -2619,17 +2897,17 @@ static int tag__recode_dwarf_type(struct tag *tag, struct cu *cu)
 					(unsigned long long)specification.off);
 			}
 		}
-		lexblock__recode_dwarf_types(&fn->lexblock, cu);
+		lexblock__recode_dwarf_types(&fn->lexblock, cu, ctx);
 	}
 		/* Fall thru */
 
 	case DW_TAG_subroutine_type:
-		ftype__recode_dwarf_types(tag, cu);
+		ftype__recode_dwarf_types(tag, cu, ctx);
 		/* Fall thru, for the function return type */
 		break;
 
 	case DW_TAG_lexical_block:
-		lexblock__recode_dwarf_types(tag__lexblock(tag), cu);
+		lexblock__recode_dwarf_types(tag__lexblock(tag), cu, ctx);
 		return 0;
 
 	case DW_TAG_ptr_to_member_type: {
@@ -2650,7 +2928,7 @@ static int tag__recode_dwarf_type(struct tag *tag, struct cu *cu)
 		break;
 
 	case DW_TAG_namespace:
-		return namespace__recode_dwarf_types(tag, cu);
+		return namespace__recode_dwarf_types(tag, cu, ctx);
 	/* Damn, DW_TAG_inlined_subroutine is an special case
            as dwarf_tag->id is in fact an abtract origin, i.e. must be
 	   looked up in the tags_table, not in the types_table.
@@ -2684,11 +2962,13 @@ static int tag__recode_dwarf_type(struct tag *tag, struct cu *cu)
 		return 0;
 	}
 
+	recode_btf_type_tags(&tag->annots, dtag->small_id, ctx);
+
 	if (dtag->type.off == 0) {
 		if (tag->tag != DW_TAG_pointer_type)
 			tag->type = 0; /* void */
 		else
-			dwarf_cu__recode_btf_type_tag_ptr(tag, 0);
+			dwarf_cu__recode_btf_type_tag_ptr(tag, 0, cu);
 		return 0;
 	}
 
@@ -2703,7 +2983,7 @@ out:
 	if (tag->tag != DW_TAG_pointer_type)
 		tag->type = dtype->small_id;
 	else
-		dwarf_cu__recode_btf_type_tag_ptr(tag, dtype->small_id);
+		dwarf_cu__recode_btf_type_tag_ptr(tag, dtype->small_id, cu);
 
 	return 0;
 }
@@ -2788,28 +3068,168 @@ static int cu__resolve_func_ret_types_optimized(struct cu *cu)
 	return 0;
 }
 
-static int cu__recode_dwarf_types_table(struct cu *cu,
-					struct ptr_table *pt,
-					uint32_t i)
+typedef int (*recode_tag_visitor)(struct tag *, struct cu*, struct recode_context *);
+
+static int cu__visit_all_tags(struct cu *cu, recode_tag_visitor fn, struct recode_context *ctx)
 {
-	for (; i < pt->nr_entries; ++i) {
-		struct tag *tag = pt->entries[i];
+	const int nr_tables = 3;
+	struct {
+		struct ptr_table *table;
+		uint32_t start_idx;
+	} tables[] = {
+		{ &cu->types_table,	1 },
+		{ &cu->tags_table,	0 },
+		{ &cu->functions_table,	0 }
+	};
+	struct ptr_table *pt;
+	struct tag *tag;
+	uint32_t i, t;
 
-		if (tag != NULL) /* void, see cu__new */
-			if (tag__recode_dwarf_type(tag, cu))
+	for (t = 0; t < nr_tables; ++t) {
+		pt = tables[t].table;
+		for (i = tables[t].start_idx; i < pt->nr_entries; ++i) {
+			tag = pt->entries[i];
+			if (!tag) /* void, see cu__new */
+				continue;
+			if (fn(tag, cu, ctx))
 				return -1;
+		}
+	}
+
+	return 0;
+}
+
+/* See comment for cu__allocate_btf_type_tags() below. */
+static int tag__allocate_btf_type_tags(struct tag *tag, struct cu *cu)
+{
+	enum annotation_kind target = cu->ignore_btf_type_tag_pointee
+				      ? BTF_TYPE_TAG
+				      : BTF_TYPE_TAG_POINTEE;
+	struct list_head *annots = &tag->annots;
+	struct llvm_annotation *annot;
+
+	list_for_each_entry(annot, annots, node) {
+		if (annot->kind != target)
+			continue;
+
+		int err = cu__assign_tag_id(cu, &annot->tag);
+		if (err)
+			return err;
 	}
 
 	return 0;
 }
 
+/* The flag `cu->ignore_btf_type_tag_pointee` is set at DWARF load phase.
+ * Before it is set it is not known what kind of type tags is used in
+ * the program, BTF_TYPE_TAG or BTF_TYPE_TAG_POINTEE.
+ * It is necessary to allocate only a single kind of tags to avoid
+ * spurious entries in the type table (and resultant BTF).
+ * Thus, the allocation of type tags is done as a separate step.
+ */
+static int cu__allocate_btf_type_tags(struct cu *cu)
+{
+	struct unspecified_type *utype;
+	struct tag *pos;
+	uint32_t id;
+	int err = 0;
+
+	cu__for_each_type(cu, id, pos) {
+		err = tag__allocate_btf_type_tags(pos, cu);
+		if (err)
+			return err;
+	}
+
+	list_for_each_entry(utype, &cu->unspecified_types, node) {
+		err = tag__allocate_btf_type_tags(&utype->tag, cu);
+		if (err)
+			return err;
+	}
+
+	return err;
+}
+
+/* `btf:type_tag` tags have special representation, when attached to void type.
+ * For example, DWARF for the following C code:
+ *
+ *   struct st {
+ *     void __attribute__(btf_type_tag("__d")) *d;
+ *   }
+ *
+ * Looks as follows:
+ *
+ *   0x29:   DW_TAG_structure_type
+ *             DW_AT_name      ("st")
+ *
+ *   0x49:     DW_TAG_member
+ *               DW_AT_name    ("d")
+ *               DW_AT_type    (0xa6 "void *")
+ *
+ *   0xa6:   DW_TAG_pointer_type
+ *             DW_AT_type      (0xaf "void")
+ *
+ *   0xaf:   DW_TAG_unspecified_type
+ *             DW_AT_name      ("void")
+ *
+ *   0xb1:     DW_TAG_LLVM_annotation
+ *               DW_AT_name    ("btf:type_tag")
+ *               DW_AT_const_value     ("__d")
+ *
+ * Here `void` type is encoded as `DW_TAG_unspecified_type` with
+ * `DW_TAG_LLVM_annotation` children.
+ *
+ * This function replaces `small_id` of the unspecified type (0xaf) with
+ * `small_id` of the first `btf:type_tag` annotation (0xb1).
+ * Thus further recode passes will use id of the type tag in place of the
+ * unspecified type id, when references to unspecified type are resolved.
+ */
+static void cu__recode_unspecified_types(struct cu *cu)
+{
+	struct btf_type_tag_mapping mapping;
+	struct unspecified_type *utype;
+	struct dwarf_tag *dtag;
+
+	list_for_each_entry(utype, &cu->unspecified_types, node) {
+		__recode_btf_type_tags(&utype->tag.annots, 0, &mapping);
+		dtag = utype->tag.priv;
+		dtag->small_id = mapping.first_tag_id;
+	}
+}
+
 static int cu__recode_dwarf_types(struct cu *cu)
 {
-	if (cu__recode_dwarf_types_table(cu, &cu->types_table, 1) ||
-	    cu__recode_dwarf_types_table(cu, &cu->tags_table, 0) ||
-	    cu__recode_dwarf_types_table(cu, &cu->functions_table, 0))
+	struct recode_context ctx = {};
+	int err = 0;
+
+	if (recode_context_init(&ctx))
 		return -1;
-	return 0;
+
+	if (cu__allocate_btf_type_tags(cu)) {
+		err = -1;
+		goto cleanup;
+	}
+
+	cu__recode_unspecified_types(cu);
+
+	if (cu__visit_all_tags(cu, tag__recode_dwarf_type, &ctx)) {
+		err = -1;
+		goto cleanup;
+	}
+
+	/* No need for second pass if there are no btf type tags */
+	if (ctx.nr_entries == 0)
+		goto cleanup;
+
+	sort_btf_type_tags(&ctx);
+
+	if (cu__visit_all_tags(cu, update_btf_type_tag_refs, &ctx)) {
+		err = -1;
+		goto cleanup;
+	}
+
+cleanup:
+	recode_context_free(&ctx);
+	return err;
 }
 
 static const char *dwarf_tag__decl_file(const struct tag *tag,
diff --git a/dwarves.h b/dwarves.h
index cbd2913..fa95267 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -261,6 +261,10 @@ struct cu {
 	uint8_t		 has_addr_info:1;
 	uint8_t		 uses_global_strings:1;
 	uint8_t		 little_endian:1;
+	/* When set means that "btf:type_tags" annotations are present
+	 * and "btf_type_tags" annotations should be ignored during export.
+	 */
+	uint8_t		 ignore_btf_type_tag_pointee:1;
 	uint8_t		 nr_register_params;
 	int		 register_params[ARCH_MAX_REGISTER_PARAMS];
 	uint16_t	 language;
@@ -628,8 +632,12 @@ static inline struct ptr_to_member_type *
 
 enum annotation_kind {
 	BTF_DECL_TAG,
-	/* "btf_type_tag" in DWARF, attached to a pointer, applies to pointee type */
+	/* "btf_type_tag" in DWARF, attached to a pointer, applies to pointee type.
+	 * Old-style encoding kept for backwards compatibility.
+	 */
 	BTF_TYPE_TAG_POINTEE,
+	/* "btf:type_tag" in DWARF, attached to any type, applies to parent type */
+	BTF_TYPE_TAG,
 };
 
 /** struct llvm_annotation - representing objects with DW_TAG_LLVM_annotation tag
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-14 23:04 ` [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute Eduard Zingerman
@ 2023-03-27 11:46   ` Arnaldo Carvalho de Melo
  2023-03-27 12:10     ` Eduard Zingerman
  2023-03-28 13:59   ` Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-27 11:46 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: dwarves, arnaldo.melo, bpf, kernel-team, ast, daniel, andrii,
	yhs, jose.marchesi, david.faust, alan.maguire

Em Wed, Mar 15, 2023 at 01:04:13AM +0200, Eduard Zingerman escreveu:
> The following example contains a structure field annotated with
> btf_type_tag attribute:
> 
>     #define __tag1 __attribute__((btf_type_tag("tag1")))
> 
>     struct st {
>       int __tag1 *a;
>     } g;
> 
> It is not printed correctly by `pahole -F dwarf` command:
> 
>     $ clang -g -c test.c -o test.o
>     pahole -F dwarf test.o
>     struct st {
>     	tag1 *                     a;                    /*     0     8 */
>     	...
>     };
> 
> Note the type for variable `a`: `tag1` is printed instead of `int`.
> This commit teaches `type__fprintf()` and `__tag_name()` logic to skip
> `DW_TAG_LLVM_annotation` objects that are used to encode type tags.

I'm applying this now to make progress on this front, but longer term we
should printf it too, as we want the output to match the original source
code as much as possible from the type information.

- Arnaldo
 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  dwarves_fprintf.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index e8399e7..1e6147a 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -572,6 +572,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>  	case DW_TAG_restrict_type:
>  	case DW_TAG_atomic_type:
>  	case DW_TAG_unspecified_type:
> +	case DW_TAG_LLVM_annotation:
>  		type = cu__type(cu, tag->type);
>  		if (type == NULL && tag->type != 0)
>  			tag__id_not_found_snprintf(bf, len, tag->type);
> @@ -786,6 +787,10 @@ next_type:
>  			n = tag__has_type_loop(type, ptype, NULL, 0, fp);
>  			if (n)
>  				return printed + n;
> +			if (ptype->tag == DW_TAG_LLVM_annotation) {
> +				type = ptype;
> +				goto next_type;
> +			}
>  			if (ptype->tag == DW_TAG_subroutine_type) {
>  				printed += ftype__fprintf(tag__ftype(ptype),
>  							  cu, name, 0, 1,
> @@ -880,6 +885,14 @@ print_modifier: {
>  		else
>  			printed += enumeration__fprintf(type, &tconf, fp);
>  		break;
> +	case DW_TAG_LLVM_annotation: {
> +		struct tag *ttype = cu__type(cu, type->type);
> +		if (ttype) {
> +			type = ttype;
> +			goto next_type;
> +		}
> +		goto out_type_not_found;
> +	}
>  	}
>  out:
>  	if (type_expanded)
> -- 
> 2.39.1
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-27 11:46   ` Arnaldo Carvalho de Melo
@ 2023-03-27 12:10     ` Eduard Zingerman
  2023-03-27 12:55       ` Arnaldo Carvalho de Melo
  2023-03-28 12:40       ` Arnaldo Carvalho de Melo
  0 siblings, 2 replies; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-27 12:10 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs,
	jose.marchesi, david.faust, alan.maguire

On Mon, 2023-03-27 at 08:46 -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 15, 2023 at 01:04:13AM +0200, Eduard Zingerman escreveu:
> > The following example contains a structure field annotated with
> > btf_type_tag attribute:
> > 
> >     #define __tag1 __attribute__((btf_type_tag("tag1")))
> > 
> >     struct st {
> >       int __tag1 *a;
> >     } g;
> > 
> > It is not printed correctly by `pahole -F dwarf` command:
> > 
> >     $ clang -g -c test.c -o test.o
> >     pahole -F dwarf test.o
> >     struct st {
> >     	tag1 *                     a;                    /*     0     8 */
> >     	...
> >     };
> > 
> > Note the type for variable `a`: `tag1` is printed instead of `int`.
> > This commit teaches `type__fprintf()` and `__tag_name()` logic to skip
> > `DW_TAG_LLVM_annotation` objects that are used to encode type tags.
> 
> I'm applying this now to make progress on this front, but longer term we
> should printf it too, as we want the output to match the original source
> code as much as possible from the type information.

Understood, thank you.

Also, I want to give a heads-up about ongoing discussion in:
https://reviews.llvm.org/D143967

The gist of the discussion is that for the code like:

  volatile __tag("foo") int;
  
Kernel expects BTF to be:

  __tag("foo") -> volatile -> int
  
And I encode it in DWARF as:

  volatile       -> int
    __tag("foo")
    
But GCC guys argue that DWARF should be like this:

  volatile       -> int
                      __tag("foo")

So, to get the BTF to a form acceptable for kernel some additional
pahole modifications might be necessary. (I will work on a prototype
for such modifications this week).

Maybe put this patch-set on-hold until that is resolved?

Thanks,
Eduard

> 
> - Arnaldo
>  
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >  dwarves_fprintf.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > index e8399e7..1e6147a 100644
> > --- a/dwarves_fprintf.c
> > +++ b/dwarves_fprintf.c
> > @@ -572,6 +572,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> >  	case DW_TAG_restrict_type:
> >  	case DW_TAG_atomic_type:
> >  	case DW_TAG_unspecified_type:
> > +	case DW_TAG_LLVM_annotation:
> >  		type = cu__type(cu, tag->type);
> >  		if (type == NULL && tag->type != 0)
> >  			tag__id_not_found_snprintf(bf, len, tag->type);
> > @@ -786,6 +787,10 @@ next_type:
> >  			n = tag__has_type_loop(type, ptype, NULL, 0, fp);
> >  			if (n)
> >  				return printed + n;
> > +			if (ptype->tag == DW_TAG_LLVM_annotation) {
> > +				type = ptype;
> > +				goto next_type;
> > +			}
> >  			if (ptype->tag == DW_TAG_subroutine_type) {
> >  				printed += ftype__fprintf(tag__ftype(ptype),
> >  							  cu, name, 0, 1,
> > @@ -880,6 +885,14 @@ print_modifier: {
> >  		else
> >  			printed += enumeration__fprintf(type, &tconf, fp);
> >  		break;
> > +	case DW_TAG_LLVM_annotation: {
> > +		struct tag *ttype = cu__type(cu, type->type);
> > +		if (ttype) {
> > +			type = ttype;
> > +			goto next_type;
> > +		}
> > +		goto out_type_not_found;
> > +	}
> >  	}
> >  out:
> >  	if (type_expanded)
> > -- 
> > 2.39.1
> > 
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-27 12:10     ` Eduard Zingerman
@ 2023-03-27 12:55       ` Arnaldo Carvalho de Melo
  2023-03-28 12:40       ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-27 12:55 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs,
	jose.marchesi, david.faust, alan.maguire



On March 27, 2023 9:10:22 AM GMT-03:00, Eduard Zingerman <eddyz87@gmail.com> wrote:
>On Mon, 2023-03-27 at 08:46 -0300, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Mar 15, 2023 at 01:04:13AM +0200, Eduard Zingerman escreveu:
>> > The following example contains a structure field annotated with
>> > btf_type_tag attribute:
>> > 
>> >     #define __tag1 __attribute__((btf_type_tag("tag1")))
>> > 
>> >     struct st {
>> >       int __tag1 *a;
>> >     } g;
>> > 
>> > It is not printed correctly by `pahole -F dwarf` command:
>> > 
>> >     $ clang -g -c test.c -o test.o
>> >     pahole -F dwarf test.o
>> >     struct st {
>> >     	tag1 *                     a;                    /*     0     8 */
>> >     	...
>> >     };
>> > 
>> > Note the type for variable `a`: `tag1` is printed instead of `int`.
>> > This commit teaches `type__fprintf()` and `__tag_name()` logic to skip
>> > `DW_TAG_LLVM_annotation` objects that are used to encode type tags.
>> 
>> I'm applying this now to make progress on this front, but longer term we
>> should printf it too, as we want the output to match the original source
>> code as much as possible from the type information.
>
>Understood, thank you.
>
>Also, I want to give a heads-up about ongoing discussion in:
>https://reviews.llvm.org/D143967
>
>The gist of the discussion is that for the code like:
>
>  volatile __tag("foo") int;
>  
>Kernel expects BTF to be:
>
>  __tag("foo") -> volatile -> int
>  
>And I encode it in DWARF as:
>
>  volatile       -> int
>    __tag("foo")
>    
>But GCC guys argue that DWARF should be like this:
>
>  volatile       -> int
>                      __tag("foo")
>
>So, to get the BTF to a form acceptable for kernel some additional
>pahole modifications might be necessary. (I will work on a prototype
>for such modifications this week).
>
>Maybe put this patch-set on-hold until that is resolved?

Ok, will read the discussion and wait,

- Arnaldo 
>
>Thanks,
>Eduard
>
>> 
>> - Arnaldo
>>  
>> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
>> > ---
>> >  dwarves_fprintf.c | 13 +++++++++++++
>> >  1 file changed, 13 insertions(+)
>> > 
>> > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
>> > index e8399e7..1e6147a 100644
>> > --- a/dwarves_fprintf.c
>> > +++ b/dwarves_fprintf.c
>> > @@ -572,6 +572,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>> >  	case DW_TAG_restrict_type:
>> >  	case DW_TAG_atomic_type:
>> >  	case DW_TAG_unspecified_type:
>> > +	case DW_TAG_LLVM_annotation:
>> >  		type = cu__type(cu, tag->type);
>> >  		if (type == NULL && tag->type != 0)
>> >  			tag__id_not_found_snprintf(bf, len, tag->type);
>> > @@ -786,6 +787,10 @@ next_type:
>> >  			n = tag__has_type_loop(type, ptype, NULL, 0, fp);
>> >  			if (n)
>> >  				return printed + n;
>> > +			if (ptype->tag == DW_TAG_LLVM_annotation) {
>> > +				type = ptype;
>> > +				goto next_type;
>> > +			}
>> >  			if (ptype->tag == DW_TAG_subroutine_type) {
>> >  				printed += ftype__fprintf(tag__ftype(ptype),
>> >  							  cu, name, 0, 1,
>> > @@ -880,6 +885,14 @@ print_modifier: {
>> >  		else
>> >  			printed += enumeration__fprintf(type, &tconf, fp);
>> >  		break;
>> > +	case DW_TAG_LLVM_annotation: {
>> > +		struct tag *ttype = cu__type(cu, type->type);
>> > +		if (ttype) {
>> > +			type = ttype;
>> > +			goto next_type;
>> > +		}
>> > +		goto out_type_not_found;
>> > +	}
>> >  	}
>> >  out:
>> >  	if (type_expanded)
>> > -- 
>> > 2.39.1
>> > 
>> 
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-27 12:10     ` Eduard Zingerman
  2023-03-27 12:55       ` Arnaldo Carvalho de Melo
@ 2023-03-28 12:40       ` Arnaldo Carvalho de Melo
  2023-03-28 13:40         ` Eduard Zingerman
  1 sibling, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-28 12:40 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Arnaldo Carvalho de Melo, dwarves, bpf, kernel-team, ast, daniel,
	andrii, yhs, jose.marchesi, david.faust, alan.maguire

Em Mon, Mar 27, 2023 at 03:10:22PM +0300, Eduard Zingerman escreveu:
> On Mon, 2023-03-27 at 08:46 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Mar 15, 2023 at 01:04:13AM +0200, Eduard Zingerman escreveu:
> > > The following example contains a structure field annotated with
> > > btf_type_tag attribute:
> > > 
> > >     #define __tag1 __attribute__((btf_type_tag("tag1")))
> > > 
> > >     struct st {
> > >       int __tag1 *a;
> > >     } g;
> > > 
> > > It is not printed correctly by `pahole -F dwarf` command:
> > > 
> > >     $ clang -g -c test.c -o test.o
> > >     pahole -F dwarf test.o
> > >     struct st {
> > >     	tag1 *                     a;                    /*     0     8 */
> > >     	...
> > >     };
> > > 
> > > Note the type for variable `a`: `tag1` is printed instead of `int`.
> > > This commit teaches `type__fprintf()` and `__tag_name()` logic to skip
> > > `DW_TAG_LLVM_annotation` objects that are used to encode type tags.
> > 
> > I'm applying this now to make progress on this front, but longer term we
> > should printf it too, as we want the output to match the original source
> > code as much as possible from the type information.
> 
> Understood, thank you.
> 
> Also, I want to give a heads-up about ongoing discussion in:
> https://reviews.llvm.org/D143967
> 
> The gist of the discussion is that for the code like:
> 
>   volatile __tag("foo") int;
>   
> Kernel expects BTF to be:
> 
>   __tag("foo") -> volatile -> int
>   
> And I encode it in DWARF as:
> 
>   volatile       -> int
>     __tag("foo")
>     
> But GCC guys argue that DWARF should be like this:
> 
>   volatile       -> int
>                       __tag("foo")
> 
> So, to get the BTF to a form acceptable for kernel some additional
> pahole modifications might be necessary. (I will work on a prototype
> for such modifications this week).
> 
> Maybe put this patch-set on-hold until that is resolved?

Ok, so I'll apply just the first two, to get btfdiff a down to those
zero sized arrays when processing clang generated DWARF for a recent
kernel, see below.

Ok?

- A rnaldo

⬢[acme@toolbox pahole]$ git log --oneline -5
b43651673676c1dc (HEAD -> master) btf_loader: A hack for BTF import of btf_type_tag attributes
e7fb771f2649fc1b fprintf: Correct names for types with btf_type_tag attribute
4d17096076b2351f (quaco/master, quaco/HEAD, github/tmp.master, github/next, acme/tmp.master, acme/next) btf_encoder: Compare functions via prototypes not parameter names
82730394195276ac fprintf: Support skipping modifier
d184aaa125ea40ff fprintf: Generalize function prototype print to support passing conf
⬢[acme@toolbox pahole]$

⬢[acme@toolbox pahole]$ btfdiff ../vmlinux-clang-pahole-1.25+rust
die__process_class: tag not supported 0x2f (template_type_parameter)!
die__process_class: tag not supported 0x33 (variant_part)!
--- /tmp/btfdiff.dwarf.EiDOTz	2023-03-28 09:38:09.283942846 -0300
+++ /tmp/btfdiff.btf.rWM9v6	2023-03-28 09:38:09.624952028 -0300
@@ -14496,7 +14496,7 @@ struct bpf_cand_cache {
 	struct {
 		const struct btf  * btf;                 /*    16     8 */
 		u32                id;                   /*    24     4 */
-	} cands[0]; /*    16     0 */
+	} cands[]; /*    16     0 */

 	/* size: 16, cachelines: 1, members: 5 */
 	/* last cacheline: 16 bytes */
@@ -18310,7 +18310,7 @@ struct btf_id_set8 {
 	struct {
 		u32                id;                   /*     8     4 */
 		u32                flags;                /*    12     4 */
-	} pairs[0]; /*     8     0 */
+	} pairs[]; /*     8     0 */

 	/* size: 8, cachelines: 1, members: 3 */
 	/* last cacheline: 8 bytes */
@@ -27765,7 +27765,7 @@ struct cpu_rmap {
 	struct {
 		u16                index;                /*    16     2 */
 		u16                dist;                 /*    18     2 */
-	} near[0]; /*    16     0 */
+	} near[]; /*    16     0 */

 	/* size: 16, cachelines: 1, members: 5 */
 	/* last cacheline: 16 bytes */
@@ -73931,7 +73931,7 @@ struct linux_efi_memreserve {
 	struct {
 		phys_addr_t        base;                 /*    16     8 */
 		phys_addr_t        size;                 /*    24     8 */
-	} entry[0]; /*    16     0 */
+	} entry[]; /*    16     0 */

 	/* size: 16, cachelines: 1, members: 4 */
 	/* last cacheline: 16 bytes */
@@ -84345,7 +84345,7 @@ struct netlink_policy_dump_state {
 	struct {
 		const struct nla_policy  * policy;       /*    16     8 */
 		unsigned int       maxtype;              /*    24     4 */
-	} policies[0]; /*    16     0 */
+	} policies[]; /*    16     0 */

 	/* size: 16, cachelines: 1, members: 4 */
 	/* sum members: 12, holes: 1, sum holes: 4 */
@@ -139178,7 +139178,7 @@ struct uv_rtc_timer_head {
 		/* XXX 4 bytes hole, try to pack */

 		u64                expires;              /*    24     8 */
-	} cpu[0]; /*    16     0 */
+	} cpu[]; /*    16     0 */

 	/* size: 16, cachelines: 1, members: 4 */
 	/* sum members: 12, holes: 1, sum holes: 4 */
⬢[acme@toolbox pahole]$

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-28 12:40       ` Arnaldo Carvalho de Melo
@ 2023-03-28 13:40         ` Eduard Zingerman
  0 siblings, 0 replies; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-28 13:40 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs,
	jose.marchesi, david.faust, alan.maguire

On Tue, 2023-03-28 at 09:40 -0300, Arnaldo Carvalho de Melo wrote:
[...]
> > Maybe put this patch-set on-hold until that is resolved?
> 
> Ok, so I'll apply just the first two, to get btfdiff a down to those
> zero sized arrays when processing clang generated DWARF for a recent
> kernel, see below.
> 
> Ok?

Sure, thank you!

[...]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-14 23:04 ` [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute Eduard Zingerman
  2023-03-27 11:46   ` Arnaldo Carvalho de Melo
@ 2023-03-28 13:59   ` Arnaldo Carvalho de Melo
  2023-03-28 14:08     ` Eduard Zingerman
  1 sibling, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-28 13:59 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: dwarves, arnaldo.melo, bpf, kernel-team, ast, daniel, andrii,
	yhs, jose.marchesi, david.faust, alan.maguire

Em Wed, Mar 15, 2023 at 01:04:13AM +0200, Eduard Zingerman escreveu:
> The following example contains a structure field annotated with
> btf_type_tag attribute:
> 
>     #define __tag1 __attribute__((btf_type_tag("tag1")))
> 
>     struct st {
>       int __tag1 *a;
>     } g;
> 
> It is not printed correctly by `pahole -F dwarf` command:
> 
>     $ clang -g -c test.c -o test.o
>     pahole -F dwarf test.o
>     struct st {
>     	tag1 *                     a;                    /*     0     8 */
>     	...
>     };
> 
> Note the type for variable `a`: `tag1` is printed instead of `int`.
> This commit teaches `type__fprintf()` and `__tag_name()` logic to skip
> `DW_TAG_LLVM_annotation` objects that are used to encode type tags.

I noticed this:

⬢[acme@toolbox pahole]$ pahole --sort -F btf ../vmlinux-clang-pahole-1.25+rust > /tmp/clang
⬢[acme@toolbox pahole]$ pahole --sort -F btf ../vmlinux-gcc-pahole-1.25+rust > /tmp/gcc


--- /tmp/gcc    2023-03-28 10:55:37.075999474 -0300
+++ /tmp/clang  2023-03-28 10:55:53.324436319 -0300
@@ -70,21 +70,21 @@
 struct Qdisc {
        int                        (*enqueue)(struct sk_buff *, struct Qdisc *, struct sk_buff * *); /*     0     8 */
        struct sk_buff *           (*dequeue)(struct Qdisc *); /*     8     8 */
        unsigned int               flags;                /*    16     4 */
        u32                        limit;                /*    20     4 */
        const struct Qdisc_ops  *  ops;                  /*    24     8 */
-       struct qdisc_size_table *  stab;                 /*    32     8 */
+       struct qdisc_size_table    stab;                 /*    32     8 */
        struct hlist_node          hash;                 /*    40    16 */
        u32                        handle;               /*    56     4 */
        u32                        parent;               /*    60     4 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        struct netdev_queue *      dev_queue;            /*    64     8 */
-       struct net_rate_estimator * rate_est;            /*    72     8 */
-       struct gnet_stats_basic_sync * cpu_bstats;       /*    80     8 */
-       struct gnet_stats_queue *  cpu_qstats;           /*    88     8 */
+       struct net_rate_estimator  rate_est;             /*    72     8 */
+       struct gnet_stats_basic_sync cpu_bstats;         /*    80     8 */
+       struct gnet_stats_queue    cpu_qstats;           /*    88     8 */
        int                        pad;                  /*    96     4 */
        refcount_t                 refcnt;               /*   100     4 */

        /* XXX 24 bytes hole, try to pack */

        /* --- cacheline 2 boundary (128 bytes) --- */

And:

struct Qdisc {
        int                     (*enqueue)(struct sk_buff *skb,
                                           struct Qdisc *sch,
                                           struct sk_buff **to_free);
        struct sk_buff *        (*dequeue)(struct Qdisc *sch);
        unsigned int            flags;
#define TCQ_F_BUILTIN           1
#define TCQ_F_INGRESS           2
#define TCQ_F_CAN_BYPASS        4
#define TCQ_F_MQROOT            8
#define TCQ_F_ONETXQUEUE        0x10 /* dequeue_skb() can assume all skbs are for
                                      * q->dev_queue : It can test
                                      * netif_xmit_frozen_or_stopped() before
                                      * dequeueing next packet.
                                      * Its true for MQ/MQPRIO slaves, or non
                                      * multiqueue device.
                                      */
#define TCQ_F_WARN_NONWC        (1 << 16)
#define TCQ_F_CPUSTATS          0x20 /* run using percpu statistics */
#define TCQ_F_NOPARENT          0x40 /* root of its hierarchy :
                                      * qdisc_tree_decrease_qlen() should stop.
                                      */
#define TCQ_F_INVISIBLE         0x80 /* invisible by default in dump */
#define TCQ_F_NOLOCK            0x100 /* qdisc does not require locking */
#define TCQ_F_OFFLOADED         0x200 /* qdisc is offloaded to HW */
        u32                     limit;
        const struct Qdisc_ops  *ops;
        struct qdisc_size_table __rcu *stab;
        struct hlist_node       hash;
        u32                     handle;
        u32                     parent;

        struct netdev_queue     *dev_queue;

        struct net_rate_estimator __rcu *rate_est;
        struct gnet_stats_basic_sync __percpu *cpu_bstats;
        struct gnet_stats_queue __percpu *cpu_qstats;
        int                     pad;
        refcount_t              refcnt;


I'll try to fix now.

- Arnaldo

 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  dwarves_fprintf.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index e8399e7..1e6147a 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -572,6 +572,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
>  	case DW_TAG_restrict_type:
>  	case DW_TAG_atomic_type:
>  	case DW_TAG_unspecified_type:
> +	case DW_TAG_LLVM_annotation:
>  		type = cu__type(cu, tag->type);
>  		if (type == NULL && tag->type != 0)
>  			tag__id_not_found_snprintf(bf, len, tag->type);
> @@ -786,6 +787,10 @@ next_type:
>  			n = tag__has_type_loop(type, ptype, NULL, 0, fp);
>  			if (n)
>  				return printed + n;
> +			if (ptype->tag == DW_TAG_LLVM_annotation) {
> +				type = ptype;
> +				goto next_type;
> +			}
>  			if (ptype->tag == DW_TAG_subroutine_type) {
>  				printed += ftype__fprintf(tag__ftype(ptype),
>  							  cu, name, 0, 1,
> @@ -880,6 +885,14 @@ print_modifier: {
>  		else
>  			printed += enumeration__fprintf(type, &tconf, fp);
>  		break;
> +	case DW_TAG_LLVM_annotation: {
> +		struct tag *ttype = cu__type(cu, type->type);
> +		if (ttype) {
> +			type = ttype;
> +			goto next_type;
> +		}
> +		goto out_type_not_found;
> +	}
>  	}
>  out:
>  	if (type_expanded)
> -- 
> 2.39.1
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-28 13:59   ` Arnaldo Carvalho de Melo
@ 2023-03-28 14:08     ` Eduard Zingerman
  2023-03-28 15:26       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-28 14:08 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs,
	jose.marchesi, david.faust, alan.maguire

On Tue, 2023-03-28 at 10:59 -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 15, 2023 at 01:04:13AM +0200, Eduard Zingerman escreveu:
> > The following example contains a structure field annotated with
> > btf_type_tag attribute:
> > 
> >     #define __tag1 __attribute__((btf_type_tag("tag1")))
> > 
> >     struct st {
> >       int __tag1 *a;
> >     } g;
> > 
> > It is not printed correctly by `pahole -F dwarf` command:
> > 
> >     $ clang -g -c test.c -o test.o
> >     pahole -F dwarf test.o
> >     struct st {
> >     	tag1 *                     a;                    /*     0     8 */
> >     	...
> >     };
> > 
> > Note the type for variable `a`: `tag1` is printed instead of `int`.
> > This commit teaches `type__fprintf()` and `__tag_name()` logic to skip
> > `DW_TAG_LLVM_annotation` objects that are used to encode type tags.
> 
> I noticed this:
> 
> ⬢[acme@toolbox pahole]$ pahole --sort -F btf ../vmlinux-clang-pahole-1.25+rust > /tmp/clang
> ⬢[acme@toolbox pahole]$ pahole --sort -F btf ../vmlinux-gcc-pahole-1.25+rust > /tmp/gcc
> 
> 
> --- /tmp/gcc    2023-03-28 10:55:37.075999474 -0300
> +++ /tmp/clang  2023-03-28 10:55:53.324436319 -0300
> @@ -70,21 +70,21 @@
>  struct Qdisc {
>         int                        (*enqueue)(struct sk_buff *, struct Qdisc *, struct sk_buff * *); /*     0     8 */
>         struct sk_buff *           (*dequeue)(struct Qdisc *); /*     8     8 */
>         unsigned int               flags;                /*    16     4 */
>         u32                        limit;                /*    20     4 */
>         const struct Qdisc_ops  *  ops;                  /*    24     8 */
> -       struct qdisc_size_table *  stab;                 /*    32     8 */
> +       struct qdisc_size_table    stab;                 /*    32     8 */
>         struct hlist_node          hash;                 /*    40    16 */
>         u32                        handle;               /*    56     4 */
>         u32                        parent;               /*    60     4 */
>         /* --- cacheline 1 boundary (64 bytes) --- */
>         struct netdev_queue *      dev_queue;            /*    64     8 */
> -       struct net_rate_estimator * rate_est;            /*    72     8 */
> -       struct gnet_stats_basic_sync * cpu_bstats;       /*    80     8 */
> -       struct gnet_stats_queue *  cpu_qstats;           /*    88     8 */
> +       struct net_rate_estimator  rate_est;             /*    72     8 */
> +       struct gnet_stats_basic_sync cpu_bstats;         /*    80     8 */
> +       struct gnet_stats_queue    cpu_qstats;           /*    88     8 */
>         int                        pad;                  /*    96     4 */
>         refcount_t                 refcnt;               /*   100     4 */
> 
>         /* XXX 24 bytes hole, try to pack */
> 
>         /* --- cacheline 2 boundary (128 bytes) --- */
> 
> And:
> 
> struct Qdisc {
>         int                     (*enqueue)(struct sk_buff *skb,
>                                            struct Qdisc *sch,
>                                            struct sk_buff **to_free);
>         struct sk_buff *        (*dequeue)(struct Qdisc *sch);
>         unsigned int            flags;
> #define TCQ_F_BUILTIN           1
> #define TCQ_F_INGRESS           2
> #define TCQ_F_CAN_BYPASS        4
> #define TCQ_F_MQROOT            8
> #define TCQ_F_ONETXQUEUE        0x10 /* dequeue_skb() can assume all skbs are for
>                                       * q->dev_queue : It can test
>                                       * netif_xmit_frozen_or_stopped() before
>                                       * dequeueing next packet.
>                                       * Its true for MQ/MQPRIO slaves, or non
>                                       * multiqueue device.
>                                       */
> #define TCQ_F_WARN_NONWC        (1 << 16)
> #define TCQ_F_CPUSTATS          0x20 /* run using percpu statistics */
> #define TCQ_F_NOPARENT          0x40 /* root of its hierarchy :
>                                       * qdisc_tree_decrease_qlen() should stop.
>                                       */
> #define TCQ_F_INVISIBLE         0x80 /* invisible by default in dump */
> #define TCQ_F_NOLOCK            0x100 /* qdisc does not require locking */
> #define TCQ_F_OFFLOADED         0x200 /* qdisc is offloaded to HW */
>         u32                     limit;
>         const struct Qdisc_ops  *ops;
>         struct qdisc_size_table __rcu *stab;
>         struct hlist_node       hash;
>         u32                     handle;
>         u32                     parent;
> 
>         struct netdev_queue     *dev_queue;
> 
>         struct net_rate_estimator __rcu *rate_est;
>         struct gnet_stats_basic_sync __percpu *cpu_bstats;
>         struct gnet_stats_queue __percpu *cpu_qstats;
>         int                     pad;
>         refcount_t              refcnt;
> 
> 
> I'll try to fix now.

Ouch. The fields are annotated with type tags, which are ignored by gcc.
If this is not urgent I can debug it either later today or tomorrow.

> 
> - Arnaldo
> 
>  
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >  dwarves_fprintf.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > index e8399e7..1e6147a 100644
> > --- a/dwarves_fprintf.c
> > +++ b/dwarves_fprintf.c
> > @@ -572,6 +572,7 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> >  	case DW_TAG_restrict_type:
> >  	case DW_TAG_atomic_type:
> >  	case DW_TAG_unspecified_type:
> > +	case DW_TAG_LLVM_annotation:
> >  		type = cu__type(cu, tag->type);
> >  		if (type == NULL && tag->type != 0)
> >  			tag__id_not_found_snprintf(bf, len, tag->type);
> > @@ -786,6 +787,10 @@ next_type:
> >  			n = tag__has_type_loop(type, ptype, NULL, 0, fp);
> >  			if (n)
> >  				return printed + n;
> > +			if (ptype->tag == DW_TAG_LLVM_annotation) {
> > +				type = ptype;
> > +				goto next_type;
> > +			}
> >  			if (ptype->tag == DW_TAG_subroutine_type) {
> >  				printed += ftype__fprintf(tag__ftype(ptype),
> >  							  cu, name, 0, 1,
> > @@ -880,6 +885,14 @@ print_modifier: {
> >  		else
> >  			printed += enumeration__fprintf(type, &tconf, fp);
> >  		break;
> > +	case DW_TAG_LLVM_annotation: {
> > +		struct tag *ttype = cu__type(cu, type->type);
> > +		if (ttype) {
> > +			type = ttype;
> > +			goto next_type;
> > +		}
> > +		goto out_type_not_found;
> > +	}
> >  	}
> >  out:
> >  	if (type_expanded)
> > -- 
> > 2.39.1
> > 
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-28 14:08     ` Eduard Zingerman
@ 2023-03-28 15:26       ` Arnaldo Carvalho de Melo
  2023-03-28 15:30         ` Eduard Zingerman
  0 siblings, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-28 15:26 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Arnaldo Carvalho de Melo, dwarves, bpf, kernel-team, ast, daniel,
	andrii, yhs, jose.marchesi, david.faust, alan.maguire

Em Tue, Mar 28, 2023 at 05:08:48PM +0300, Eduard Zingerman escreveu:
> On Tue, 2023-03-28 at 10:59 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Mar 15, 2023 at 01:04:13AM +0200, Eduard Zingerman escreveu:
> > > The following example contains a structure field annotated with
> > > btf_type_tag attribute:
> > > 
> > >     #define __tag1 __attribute__((btf_type_tag("tag1")))
> > > 
> > >     struct st {
> > >       int __tag1 *a;
> > >     } g;
> > > 
> > > It is not printed correctly by `pahole -F dwarf` command:
> > > 
> > >     $ clang -g -c test.c -o test.o
> > >     pahole -F dwarf test.o
> > >     struct st {
> > >     	tag1 *                     a;                    /*     0     8 */
> > >     	...
> > >     };
> > > 
> > > Note the type for variable `a`: `tag1` is printed instead of `int`.
> > > This commit teaches `type__fprintf()` and `__tag_name()` logic to skip
> > > `DW_TAG_LLVM_annotation` objects that are used to encode type tags.
> > 
> > I noticed this:
> > 
> > ⬢[acme@toolbox pahole]$ pahole --sort -F btf ../vmlinux-clang-pahole-1.25+rust > /tmp/clang
> > ⬢[acme@toolbox pahole]$ pahole --sort -F btf ../vmlinux-gcc-pahole-1.25+rust > /tmp/gcc
> > 
> > 
> > --- /tmp/gcc    2023-03-28 10:55:37.075999474 -0300
> > +++ /tmp/clang  2023-03-28 10:55:53.324436319 -0300
> > @@ -70,21 +70,21 @@
> >  struct Qdisc {
> >         int                        (*enqueue)(struct sk_buff *, struct Qdisc *, struct sk_buff * *); /*     0     8 */
> >         struct sk_buff *           (*dequeue)(struct Qdisc *); /*     8     8 */
> >         unsigned int               flags;                /*    16     4 */
> >         u32                        limit;                /*    20     4 */
> >         const struct Qdisc_ops  *  ops;                  /*    24     8 */
> > -       struct qdisc_size_table *  stab;                 /*    32     8 */
> > +       struct qdisc_size_table    stab;                 /*    32     8 */
> >         struct hlist_node          hash;                 /*    40    16 */
> >         u32                        handle;               /*    56     4 */
> >         u32                        parent;               /*    60     4 */
> >         /* --- cacheline 1 boundary (64 bytes) --- */
> >         struct netdev_queue *      dev_queue;            /*    64     8 */
> > -       struct net_rate_estimator * rate_est;            /*    72     8 */
> > -       struct gnet_stats_basic_sync * cpu_bstats;       /*    80     8 */
> > -       struct gnet_stats_queue *  cpu_qstats;           /*    88     8 */
> > +       struct net_rate_estimator  rate_est;             /*    72     8 */
> > +       struct gnet_stats_basic_sync cpu_bstats;         /*    80     8 */
> > +       struct gnet_stats_queue    cpu_qstats;           /*    88     8 */
> >         int                        pad;                  /*    96     4 */
> >         refcount_t                 refcnt;               /*   100     4 */
> > 
> >         /* XXX 24 bytes hole, try to pack */
> > 
> >         /* --- cacheline 2 boundary (128 bytes) --- */
> > 
> > And:
> > 
> > struct Qdisc {
> >         int                     (*enqueue)(struct sk_buff *skb,
> >                                            struct Qdisc *sch,
> >                                            struct sk_buff **to_free);
> >         struct sk_buff *        (*dequeue)(struct Qdisc *sch);
> >         unsigned int            flags;
> > #define TCQ_F_BUILTIN           1
> > #define TCQ_F_INGRESS           2
> > #define TCQ_F_CAN_BYPASS        4
> > #define TCQ_F_MQROOT            8
> > #define TCQ_F_ONETXQUEUE        0x10 /* dequeue_skb() can assume all skbs are for
> >                                       * q->dev_queue : It can test
> >                                       * netif_xmit_frozen_or_stopped() before
> >                                       * dequeueing next packet.
> >                                       * Its true for MQ/MQPRIO slaves, or non
> >                                       * multiqueue device.
> >                                       */
> > #define TCQ_F_WARN_NONWC        (1 << 16)
> > #define TCQ_F_CPUSTATS          0x20 /* run using percpu statistics */
> > #define TCQ_F_NOPARENT          0x40 /* root of its hierarchy :
> >                                       * qdisc_tree_decrease_qlen() should stop.
> >                                       */
> > #define TCQ_F_INVISIBLE         0x80 /* invisible by default in dump */
> > #define TCQ_F_NOLOCK            0x100 /* qdisc does not require locking */
> > #define TCQ_F_OFFLOADED         0x200 /* qdisc is offloaded to HW */
> >         u32                     limit;
> >         const struct Qdisc_ops  *ops;
> >         struct qdisc_size_table __rcu *stab;
> >         struct hlist_node       hash;
> >         u32                     handle;
> >         u32                     parent;
> > 
> >         struct netdev_queue     *dev_queue;
> > 
> >         struct net_rate_estimator __rcu *rate_est;
> >         struct gnet_stats_basic_sync __percpu *cpu_bstats;
> >         struct gnet_stats_queue __percpu *cpu_qstats;
> >         int                     pad;
> >         refcount_t              refcnt;
> > 
> > 
> > I'll try to fix now.
> 
> Ouch. The fields are annotated with type tags, which are ignored by gcc.
> If this is not urgent I can debug it either later today or tomorrow.

Sure, but look:

> > -       struct qdisc_size_table *  stab;                 /*    32     8 */
> > +       struct qdisc_size_table    stab;                 /*    32     8 */

Its the DW_TAG_pointer_type that is getting lost somehow:

 <1><b0af32>: Abbrev Number: 81 (DW_TAG_structure_type)
    <b0af33>   DW_AT_name        : (indirect string, offset: 0xe825): Qdisc
    <b0af37>   DW_AT_byte_size   : 384
    <b0af39>   DW_AT_decl_file   : 223
    <b0af3a>   DW_AT_decl_line   : 72

<SNIP>

 <2><b0af77>: Abbrev Number: 65 (DW_TAG_member)
    <b0af78>   DW_AT_name        : (indirect string, offset: 0x4745ff): stab
    <b0af7c>   DW_AT_type        : <0xb0b76b>
    <b0af80>   DW_AT_decl_file   : 223
    <b0af81>   DW_AT_decl_line   : 99
    <b0af82>   DW_AT_data_member_location: 32

<SNIP>

<1><b0b76b>: Abbrev Number: 61 (DW_TAG_pointer_type)
    <b0b76c>   DW_AT_type        : <0xb0b77a>
 <2><b0b770>: Abbrev Number: 62 (User TAG value: 0x6000)
    <b0b771>   DW_AT_name        : (indirect string, offset: 0x378): btf_type_tag
    <b0b775>   DW_AT_const_value : (indirect string, offset: 0x6e93): rcu
 <2><b0b779>: Abbrev Number: 0
 <1><b0b77a>: Abbrev Number: 69 (DW_TAG_structure_type)
    <b0b77b>   DW_AT_name        : (indirect string, offset: 0x6e5ed): qdisc_size_table
    <b0b77f>   DW_AT_byte_size   : 64
    <b0b780>   DW_AT_decl_file   : 223
    <b0b781>   DW_AT_decl_line   : 56

 
So it's all there in the DWARF info:

   b0af77 has type 0xb0b76b which is a DW_TAG_pointer_type that has type
   0xb0b77a, that is DW_TAG_structure_type.

Now lets see how this was encoded into BTF:

[4725] STRUCT 'Qdisc' size=384 vlen=28
<SNIP>
        'stab' type_id=4790 bits_offset=256
<SNIP>
[4790] PTR '(anon)' type_id=4789
<SNIP>
[4789] TYPE_TAG 'rcu' type_id=4791
<SNIP>
[4791] STRUCT 'qdisc_size_table' size=64 vlen=5
        'rcu' type_id=320 bits_offset=0
        'list' type_id=87 bits_offset=128
        'szopts' type_id=4792 bits_offset=256
        'refcnt' type_id=16 bits_offset=448
        'data' type_id=4659 bits_offset=480

So it all seems ok, we should keep all the info and teach fprintf to
handle TYPE_TAG.

Which you tried, but somehow the '*' link is missing.

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-28 15:26       ` Arnaldo Carvalho de Melo
@ 2023-03-28 15:30         ` Eduard Zingerman
  2023-03-28 21:17           ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-28 15:30 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs,
	jose.marchesi, david.faust, alan.maguire

On Tue, 2023-03-28 at 12:26 -0300, Arnaldo Carvalho de Melo wrote:
[...] 
> Sure, but look:
> 
> > > -       struct qdisc_size_table *  stab;                 /*    32     8 */
> > > +       struct qdisc_size_table    stab;                 /*    32     8 */
> 
> Its the DW_TAG_pointer_type that is getting lost somehow:
> 
>  <1><b0af32>: Abbrev Number: 81 (DW_TAG_structure_type)
>     <b0af33>   DW_AT_name        : (indirect string, offset: 0xe825): Qdisc
>     <b0af37>   DW_AT_byte_size   : 384
>     <b0af39>   DW_AT_decl_file   : 223
>     <b0af3a>   DW_AT_decl_line   : 72
> 
> <SNIP>
> 
>  <2><b0af77>: Abbrev Number: 65 (DW_TAG_member)
>     <b0af78>   DW_AT_name        : (indirect string, offset: 0x4745ff): stab
>     <b0af7c>   DW_AT_type        : <0xb0b76b>
>     <b0af80>   DW_AT_decl_file   : 223
>     <b0af81>   DW_AT_decl_line   : 99
>     <b0af82>   DW_AT_data_member_location: 32
> 
> <SNIP>
> 
> <1><b0b76b>: Abbrev Number: 61 (DW_TAG_pointer_type)
>     <b0b76c>   DW_AT_type        : <0xb0b77a>
>  <2><b0b770>: Abbrev Number: 62 (User TAG value: 0x6000)
>     <b0b771>   DW_AT_name        : (indirect string, offset: 0x378): btf_type_tag
>     <b0b775>   DW_AT_const_value : (indirect string, offset: 0x6e93): rcu
>  <2><b0b779>: Abbrev Number: 0
>  <1><b0b77a>: Abbrev Number: 69 (DW_TAG_structure_type)
>     <b0b77b>   DW_AT_name        : (indirect string, offset: 0x6e5ed): qdisc_size_table
>     <b0b77f>   DW_AT_byte_size   : 64
>     <b0b780>   DW_AT_decl_file   : 223
>     <b0b781>   DW_AT_decl_line   : 56
> 
>  
> So it's all there in the DWARF info:
> 
>    b0af77 has type 0xb0b76b which is a DW_TAG_pointer_type that has type
>    0xb0b77a, that is DW_TAG_structure_type.
> 
> Now lets see how this was encoded into BTF:
> 
> [4725] STRUCT 'Qdisc' size=384 vlen=28
> <SNIP>
>         'stab' type_id=4790 bits_offset=256
> <SNIP>
> [4790] PTR '(anon)' type_id=4789
> <SNIP>
> [4789] TYPE_TAG 'rcu' type_id=4791
> <SNIP>
> [4791] STRUCT 'qdisc_size_table' size=64 vlen=5
>         'rcu' type_id=320 bits_offset=0
>         'list' type_id=87 bits_offset=128
>         'szopts' type_id=4792 bits_offset=256
>         'refcnt' type_id=16 bits_offset=448
>         'data' type_id=4659 bits_offset=480
> 
> So it all seems ok, we should keep all the info and teach fprintf to
> handle TYPE_TAG.
> 
> Which you tried, but somehow the '*' link is missing.

Yep, thanks a lot for the analysis, I will take a look.
Hopefully will send v2 tomorrow.

> 
> - Arnaldo


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-28 15:30         ` Eduard Zingerman
@ 2023-03-28 21:17           ` Arnaldo Carvalho de Melo
  2023-03-29 15:36             ` Eduard Zingerman
  0 siblings, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-28 21:17 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Arnaldo Carvalho de Melo, dwarves, bpf, kernel-team, ast, daniel,
	andrii, yhs, jose.marchesi, david.faust, alan.maguire

Em Tue, Mar 28, 2023 at 06:30:05PM +0300, Eduard Zingerman escreveu:
> On Tue, 2023-03-28 at 12:26 -0300, Arnaldo Carvalho de Melo wrote:
> [...] 
> > Sure, but look:
> > 
> > > > -       struct qdisc_size_table *  stab;                 /*    32     8 */
> > > > +       struct qdisc_size_table    stab;                 /*    32     8 */
> > 
> > Its the DW_TAG_pointer_type that is getting lost somehow:
> > 
> >  <1><b0af32>: Abbrev Number: 81 (DW_TAG_structure_type)
> >     <b0af33>   DW_AT_name        : (indirect string, offset: 0xe825): Qdisc
> >     <b0af37>   DW_AT_byte_size   : 384
> >     <b0af39>   DW_AT_decl_file   : 223
> >     <b0af3a>   DW_AT_decl_line   : 72
> > 
> > <SNIP>
> > 
> >  <2><b0af77>: Abbrev Number: 65 (DW_TAG_member)
> >     <b0af78>   DW_AT_name        : (indirect string, offset: 0x4745ff): stab
> >     <b0af7c>   DW_AT_type        : <0xb0b76b>
> >     <b0af80>   DW_AT_decl_file   : 223
> >     <b0af81>   DW_AT_decl_line   : 99
> >     <b0af82>   DW_AT_data_member_location: 32
> > 
> > <SNIP>
> > 
> > <1><b0b76b>: Abbrev Number: 61 (DW_TAG_pointer_type)
> >     <b0b76c>   DW_AT_type        : <0xb0b77a>
> >  <2><b0b770>: Abbrev Number: 62 (User TAG value: 0x6000)
> >     <b0b771>   DW_AT_name        : (indirect string, offset: 0x378): btf_type_tag
> >     <b0b775>   DW_AT_const_value : (indirect string, offset: 0x6e93): rcu
> >  <2><b0b779>: Abbrev Number: 0
> >  <1><b0b77a>: Abbrev Number: 69 (DW_TAG_structure_type)
> >     <b0b77b>   DW_AT_name        : (indirect string, offset: 0x6e5ed): qdisc_size_table
> >     <b0b77f>   DW_AT_byte_size   : 64
> >     <b0b780>   DW_AT_decl_file   : 223
> >     <b0b781>   DW_AT_decl_line   : 56
> > 
> >  
> > So it's all there in the DWARF info:
> > 
> >    b0af77 has type 0xb0b76b which is a DW_TAG_pointer_type that has type
> >    0xb0b77a, that is DW_TAG_structure_type.
> > 
> > Now lets see how this was encoded into BTF:
> > 
> > [4725] STRUCT 'Qdisc' size=384 vlen=28
> > <SNIP>
> >         'stab' type_id=4790 bits_offset=256
> > <SNIP>
> > [4790] PTR '(anon)' type_id=4789
> > <SNIP>
> > [4789] TYPE_TAG 'rcu' type_id=4791
> > <SNIP>
> > [4791] STRUCT 'qdisc_size_table' size=64 vlen=5
> >         'rcu' type_id=320 bits_offset=0
> >         'list' type_id=87 bits_offset=128
> >         'szopts' type_id=4792 bits_offset=256
> >         'refcnt' type_id=16 bits_offset=448
> >         'data' type_id=4659 bits_offset=480
> > 
> > So it all seems ok, we should keep all the info and teach fprintf to
> > handle TYPE_TAG.
> > 
> > Which you tried, but somehow the '*' link is missing.
> 
> Yep, thanks a lot for the analysis, I will take a look.
> Hopefully will send v2 tomorrow.

So, with the patch below it gets equivalent, but some more tweaking is
needed to make the output completely match (spaces, "long usigned" ->
"unsigned long", which seems to be all equivalent):

--- /tmp/gcc    2023-03-28 18:13:42.022385428 -0300
+++ /tmp/clang  2023-03-28 18:13:45.854486885 -0300
@@ -73,15 +73,15 @@
        unsigned int               flags;                /*    16     4 */
        u32                        limit;                /*    20     4 */
        const struct Qdisc_ops  *  ops;                  /*    24     8 */
-       struct qdisc_size_table *  stab;                 /*    32     8 */
+       struct qdisc_size_table  * stab;                 /*    32     8 */
        struct hlist_node          hash;                 /*    40    16 */
        u32                        handle;               /*    56     4 */
        u32                        parent;               /*    60     4 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        struct netdev_queue *      dev_queue;            /*    64     8 */
-       struct net_rate_estimator * rate_est;            /*    72     8 */
-       struct gnet_stats_basic_sync * cpu_bstats;       /*    80     8 */
-       struct gnet_stats_queue *  cpu_qstats;           /*    88     8 */
+       struct net_rate_estimator  * rate_est;           /*    72     8 */
+       struct gnet_stats_basic_sync  * cpu_bstats;      /*    80     8 */
+       struct gnet_stats_queue  * cpu_qstats;           /*    88     8 */
        int                        pad;                  /*    96     4 */
        refcount_t                 refcnt;               /*   100     4 */

@@ -96,8 +96,8 @@

        /* XXX 4 bytes hole, try to pack */

-       long unsigned int          state;                /*   216     8 */
-       long unsigned int          state2;               /*   224     8 */
+       unsigned long              state;                /*   216     8 */
+       unsigned long              state2;               /*   224     8 */
        struct Qdisc *             next_sched;           /*   232     8 */
        struct sk_buff_head        skb_bad_txq;          /*   240    24 */

@@ -112,7 +112,7 @@
        /* XXX 40 bytes hole, try to pack */

        /* --- cacheline 6 boundary (384 bytes) --- */
-       long int                   privdata[];           /*   384     0 */
+       long                       privdata[];           /*   384     0 */

        /* size: 384, cachelines: 6, members: 28 */
        /* sum members: 260, holes: 4, sum holes: 124 */
@@ -145,19 +145,19 @@
        /* XXX 4 bytes hole, try to pack */

        struct netdev_queue *      (*select_queue)(struct Qdisc *, struct tcmsg *); /*     8     8 */
-       int                        (*graft)(struct Qdisc *, long unsigned int, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /*    16     8 */
+       int                        (*graft)(struct Qdisc *, unsigned long, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /*    16     8 */
-       struct Qdisc *             (*leaf)(struct Qdisc *, long unsigned int); /*    24     8 */
+       struct Qdisc *             (*leaf)(struct Qdisc *, unsigned long); /*    24     8 */
-       void                       (*qlen_notify)(struct Qdisc *, long unsigned int); /*    32     8 */
+       void                       (*qlen_notify)(struct Qdisc *, unsigned long); /*    32     8 */
-       long unsigned int          (*find)(struct Qdisc *, u32); /*    40     8 */
+       unsigned long              (*find)(struct Qdisc *, u32); /*    40     8 */
-       int                        (*change)(struct Qdisc *, u32, u32, struct nlattr * *, long unsigned int *, struct netlink_ext_ack *); /*    48     8 */
+       int                        (*change)(struct Qdisc *, u32, u32, struct nlattr * *, unsigned long *, struct netlink_ext_ack *); /*    48     8 */
-       int                        (*delete)(struct Qdisc *, long unsigned int, struct netlink_ext_ack *); /*    56     8 */
+       int                        (*delete)(struct Qdisc *, unsigned long, struct netlink_ext_ack *); /*    56     8 */

diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index 1e6147a82056c188..1ecc24321bf8f975 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -788,8 +788,15 @@ next_type:
 			if (n)
 				return printed + n;
 			if (ptype->tag == DW_TAG_LLVM_annotation) {
-				type = ptype;
-				goto next_type;
+				// FIXME: Just skip this for now, we need to print this annotation
+				// to match the original source code.
+
+				if (ptype->type == 0) {
+					printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name);
+					break;
+				}
+
+				ptype = cu__type(cu, ptype->type);
 			}
 			if (ptype->tag == DW_TAG_subroutine_type) {
 				printed += ftype__fprintf(tag__ftype(ptype),

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-28 21:17           ` Arnaldo Carvalho de Melo
@ 2023-03-29 15:36             ` Eduard Zingerman
  2023-03-29 15:43               ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-29 15:36 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs,
	jose.marchesi, david.faust, alan.maguire

On Tue, 2023-03-28 at 18:17 -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 28, 2023 at 06:30:05PM +0300, Eduard Zingerman escreveu:
> > On Tue, 2023-03-28 at 12:26 -0300, Arnaldo Carvalho de Melo wrote:
> > [...] 
> > > Sure, but look:
> > > 
> > > > > -       struct qdisc_size_table *  stab;                 /*    32     8 */
> > > > > +       struct qdisc_size_table    stab;                 /*    32     8 */
> > > 
> > > Its the DW_TAG_pointer_type that is getting lost somehow:
> > > 
> > >  <1><b0af32>: Abbrev Number: 81 (DW_TAG_structure_type)
> > >     <b0af33>   DW_AT_name        : (indirect string, offset: 0xe825): Qdisc
> > >     <b0af37>   DW_AT_byte_size   : 384
> > >     <b0af39>   DW_AT_decl_file   : 223
> > >     <b0af3a>   DW_AT_decl_line   : 72
> > > 
> > > <SNIP>
> > > 
> > >  <2><b0af77>: Abbrev Number: 65 (DW_TAG_member)
> > >     <b0af78>   DW_AT_name        : (indirect string, offset: 0x4745ff): stab
> > >     <b0af7c>   DW_AT_type        : <0xb0b76b>
> > >     <b0af80>   DW_AT_decl_file   : 223
> > >     <b0af81>   DW_AT_decl_line   : 99
> > >     <b0af82>   DW_AT_data_member_location: 32
> > > 
> > > <SNIP>
> > > 
> > > <1><b0b76b>: Abbrev Number: 61 (DW_TAG_pointer_type)
> > >     <b0b76c>   DW_AT_type        : <0xb0b77a>
> > >  <2><b0b770>: Abbrev Number: 62 (User TAG value: 0x6000)
> > >     <b0b771>   DW_AT_name        : (indirect string, offset: 0x378): btf_type_tag
> > >     <b0b775>   DW_AT_const_value : (indirect string, offset: 0x6e93): rcu
> > >  <2><b0b779>: Abbrev Number: 0
> > >  <1><b0b77a>: Abbrev Number: 69 (DW_TAG_structure_type)
> > >     <b0b77b>   DW_AT_name        : (indirect string, offset: 0x6e5ed): qdisc_size_table
> > >     <b0b77f>   DW_AT_byte_size   : 64
> > >     <b0b780>   DW_AT_decl_file   : 223
> > >     <b0b781>   DW_AT_decl_line   : 56
> > > 
> > >  
> > > So it's all there in the DWARF info:
> > > 
> > >    b0af77 has type 0xb0b76b which is a DW_TAG_pointer_type that has type
> > >    0xb0b77a, that is DW_TAG_structure_type.
> > > 
> > > Now lets see how this was encoded into BTF:
> > > 
> > > [4725] STRUCT 'Qdisc' size=384 vlen=28
> > > <SNIP>
> > >         'stab' type_id=4790 bits_offset=256
> > > <SNIP>
> > > [4790] PTR '(anon)' type_id=4789
> > > <SNIP>
> > > [4789] TYPE_TAG 'rcu' type_id=4791
> > > <SNIP>
> > > [4791] STRUCT 'qdisc_size_table' size=64 vlen=5
> > >         'rcu' type_id=320 bits_offset=0
> > >         'list' type_id=87 bits_offset=128
> > >         'szopts' type_id=4792 bits_offset=256
> > >         'refcnt' type_id=16 bits_offset=448
> > >         'data' type_id=4659 bits_offset=480
> > > 
> > > So it all seems ok, we should keep all the info and teach fprintf to
> > > handle TYPE_TAG.
> > > 
> > > Which you tried, but somehow the '*' link is missing.
> > 
> > Yep, thanks a lot for the analysis, I will take a look.
> > Hopefully will send v2 tomorrow.
> 
> So, with the patch below it gets equivalent, but some more tweaking is
> needed to make the output completely match (spaces, "long usigned" ->
> "unsigned long", which seems to be all equivalent):
> 
> --- /tmp/gcc    2023-03-28 18:13:42.022385428 -0300
> +++ /tmp/clang  2023-03-28 18:13:45.854486885 -0300
> @@ -73,15 +73,15 @@
>         unsigned int               flags;                /*    16     4 */
>         u32                        limit;                /*    20     4 */
>         const struct Qdisc_ops  *  ops;                  /*    24     8 */
> -       struct qdisc_size_table *  stab;                 /*    32     8 */
> +       struct qdisc_size_table  * stab;                 /*    32     8 */
>         struct hlist_node          hash;                 /*    40    16 */
>         u32                        handle;               /*    56     4 */
>         u32                        parent;               /*    60     4 */
>         /* --- cacheline 1 boundary (64 bytes) --- */
>         struct netdev_queue *      dev_queue;            /*    64     8 */
> -       struct net_rate_estimator * rate_est;            /*    72     8 */
> -       struct gnet_stats_basic_sync * cpu_bstats;       /*    80     8 */
> -       struct gnet_stats_queue *  cpu_qstats;           /*    88     8 */
> +       struct net_rate_estimator  * rate_est;           /*    72     8 */
> +       struct gnet_stats_basic_sync  * cpu_bstats;      /*    80     8 */
> +       struct gnet_stats_queue  * cpu_qstats;           /*    88     8 */
>         int                        pad;                  /*    96     4 */
>         refcount_t                 refcnt;               /*   100     4 */
> 
> @@ -96,8 +96,8 @@
> 
>         /* XXX 4 bytes hole, try to pack */
> 
> -       long unsigned int          state;                /*   216     8 */
> -       long unsigned int          state2;               /*   224     8 */
> +       unsigned long              state;                /*   216     8 */
> +       unsigned long              state2;               /*   224     8 */
>         struct Qdisc *             next_sched;           /*   232     8 */
>         struct sk_buff_head        skb_bad_txq;          /*   240    24 */
> 
> @@ -112,7 +112,7 @@
>         /* XXX 40 bytes hole, try to pack */
> 
>         /* --- cacheline 6 boundary (384 bytes) --- */
> -       long int                   privdata[];           /*   384     0 */
> +       long                       privdata[];           /*   384     0 */
> 
>         /* size: 384, cachelines: 6, members: 28 */
>         /* sum members: 260, holes: 4, sum holes: 124 */
> @@ -145,19 +145,19 @@
>         /* XXX 4 bytes hole, try to pack */
> 
>         struct netdev_queue *      (*select_queue)(struct Qdisc *, struct tcmsg *); /*     8     8 */
> -       int                        (*graft)(struct Qdisc *, long unsigned int, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /*    16     8 */
> +       int                        (*graft)(struct Qdisc *, unsigned long, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /*    16     8 */
> -       struct Qdisc *             (*leaf)(struct Qdisc *, long unsigned int); /*    24     8 */
> +       struct Qdisc *             (*leaf)(struct Qdisc *, unsigned long); /*    24     8 */
> -       void                       (*qlen_notify)(struct Qdisc *, long unsigned int); /*    32     8 */
> +       void                       (*qlen_notify)(struct Qdisc *, unsigned long); /*    32     8 */
> -       long unsigned int          (*find)(struct Qdisc *, u32); /*    40     8 */
> +       unsigned long              (*find)(struct Qdisc *, u32); /*    40     8 */
> -       int                        (*change)(struct Qdisc *, u32, u32, struct nlattr * *, long unsigned int *, struct netlink_ext_ack *); /*    48     8 */
> +       int                        (*change)(struct Qdisc *, u32, u32, struct nlattr * *, unsigned long *, struct netlink_ext_ack *); /*    48     8 */
> -       int                        (*delete)(struct Qdisc *, long unsigned int, struct netlink_ext_ack *); /*    56     8 */
> +       int                        (*delete)(struct Qdisc *, unsigned long, struct netlink_ext_ack *); /*    56     8 */
> 
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index 1e6147a82056c188..1ecc24321bf8f975 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -788,8 +788,15 @@ next_type:
>  			if (n)
>  				return printed + n;
>  			if (ptype->tag == DW_TAG_LLVM_annotation) {
> -				type = ptype;
> -				goto next_type;
> +				// FIXME: Just skip this for now, we need to print this annotation
> +				// to match the original source code.
> +
> +				if (ptype->type == 0) {
> +					printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name);
> +					break;
> +				}
> +
> +				ptype = cu__type(cu, ptype->type);
>  			}
>  			if (ptype->tag == DW_TAG_subroutine_type) {
>  				printed += ftype__fprintf(tag__ftype(ptype),

This explains why '*' was missing, but unfortunately it breaks apart
when there are multiple type tags, e.g.:


    $ cat tag-test.c
    #define __t __attribute__((btf_type_tag("t1")))
    
    struct foo {
      int  (__t __t *a)(void);
    } g;
    $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
    struct foo {
    	int ()(void)   *           a;                    /*     0     8 */
    
    	/* size: 8, cachelines: 1, members: 1 */
    	/* last cacheline: 8 bytes */
    };
    $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o
    struct foo {
    	int ()(void)   *           a;                    /*     0     8 */
    
    	/* size: 8, cachelines: 1, members: 1 */
    	/* last cacheline: 8 bytes */
    };
    
What I don't understand is why pointer's type is LLVM_annotation.
Probably, the cleanest solution would be to make DWARF and BTF loaders
work in a similar way and attach LLVM_annotation as `annots` field of
the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's
in the middle of type chains. I'll work on this.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-29 15:36             ` Eduard Zingerman
@ 2023-03-29 15:43               ` Arnaldo Carvalho de Melo
  2023-03-29 16:02                 ` Eduard Zingerman
  0 siblings, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-29 15:43 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Arnaldo Carvalho de Melo, dwarves, bpf, kernel-team, ast, daniel,
	andrii, yhs, jose.marchesi, david.faust, alan.maguire

Em Wed, Mar 29, 2023 at 06:36:34PM +0300, Eduard Zingerman escreveu:
> On Tue, 2023-03-28 at 18:17 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Mar 28, 2023 at 06:30:05PM +0300, Eduard Zingerman escreveu:
> > > On Tue, 2023-03-28 at 12:26 -0300, Arnaldo Carvalho de Melo wrote:
> > > [...] 
> > > > Sure, but look:
> > > > 
> > > > > > -       struct qdisc_size_table *  stab;                 /*    32     8 */
> > > > > > +       struct qdisc_size_table    stab;                 /*    32     8 */
> > > > 
> > > > Its the DW_TAG_pointer_type that is getting lost somehow:
> > > > 
> > > >  <1><b0af32>: Abbrev Number: 81 (DW_TAG_structure_type)
> > > >     <b0af33>   DW_AT_name        : (indirect string, offset: 0xe825): Qdisc
> > > >     <b0af37>   DW_AT_byte_size   : 384
> > > >     <b0af39>   DW_AT_decl_file   : 223
> > > >     <b0af3a>   DW_AT_decl_line   : 72
> > > > 
> > > > <SNIP>
> > > > 
> > > >  <2><b0af77>: Abbrev Number: 65 (DW_TAG_member)
> > > >     <b0af78>   DW_AT_name        : (indirect string, offset: 0x4745ff): stab
> > > >     <b0af7c>   DW_AT_type        : <0xb0b76b>
> > > >     <b0af80>   DW_AT_decl_file   : 223
> > > >     <b0af81>   DW_AT_decl_line   : 99
> > > >     <b0af82>   DW_AT_data_member_location: 32
> > > > 
> > > > <SNIP>
> > > > 
> > > > <1><b0b76b>: Abbrev Number: 61 (DW_TAG_pointer_type)
> > > >     <b0b76c>   DW_AT_type        : <0xb0b77a>
> > > >  <2><b0b770>: Abbrev Number: 62 (User TAG value: 0x6000)
> > > >     <b0b771>   DW_AT_name        : (indirect string, offset: 0x378): btf_type_tag
> > > >     <b0b775>   DW_AT_const_value : (indirect string, offset: 0x6e93): rcu
> > > >  <2><b0b779>: Abbrev Number: 0
> > > >  <1><b0b77a>: Abbrev Number: 69 (DW_TAG_structure_type)
> > > >     <b0b77b>   DW_AT_name        : (indirect string, offset: 0x6e5ed): qdisc_size_table
> > > >     <b0b77f>   DW_AT_byte_size   : 64
> > > >     <b0b780>   DW_AT_decl_file   : 223
> > > >     <b0b781>   DW_AT_decl_line   : 56
> > > > 
> > > >  
> > > > So it's all there in the DWARF info:
> > > > 
> > > >    b0af77 has type 0xb0b76b which is a DW_TAG_pointer_type that has type
> > > >    0xb0b77a, that is DW_TAG_structure_type.
> > > > 
> > > > Now lets see how this was encoded into BTF:
> > > > 
> > > > [4725] STRUCT 'Qdisc' size=384 vlen=28
> > > > <SNIP>
> > > >         'stab' type_id=4790 bits_offset=256
> > > > <SNIP>
> > > > [4790] PTR '(anon)' type_id=4789
> > > > <SNIP>
> > > > [4789] TYPE_TAG 'rcu' type_id=4791
> > > > <SNIP>
> > > > [4791] STRUCT 'qdisc_size_table' size=64 vlen=5
> > > >         'rcu' type_id=320 bits_offset=0
> > > >         'list' type_id=87 bits_offset=128
> > > >         'szopts' type_id=4792 bits_offset=256
> > > >         'refcnt' type_id=16 bits_offset=448
> > > >         'data' type_id=4659 bits_offset=480
> > > > 
> > > > So it all seems ok, we should keep all the info and teach fprintf to
> > > > handle TYPE_TAG.
> > > > 
> > > > Which you tried, but somehow the '*' link is missing.
> > > 
> > > Yep, thanks a lot for the analysis, I will take a look.
> > > Hopefully will send v2 tomorrow.
> > 
> > So, with the patch below it gets equivalent, but some more tweaking is
> > needed to make the output completely match (spaces, "long usigned" ->
> > "unsigned long", which seems to be all equivalent):
> > 
> > --- /tmp/gcc    2023-03-28 18:13:42.022385428 -0300
> > +++ /tmp/clang  2023-03-28 18:13:45.854486885 -0300
> > @@ -73,15 +73,15 @@
> >         unsigned int               flags;                /*    16     4 */
> >         u32                        limit;                /*    20     4 */
> >         const struct Qdisc_ops  *  ops;                  /*    24     8 */
> > -       struct qdisc_size_table *  stab;                 /*    32     8 */
> > +       struct qdisc_size_table  * stab;                 /*    32     8 */
> >         struct hlist_node          hash;                 /*    40    16 */
> >         u32                        handle;               /*    56     4 */
> >         u32                        parent;               /*    60     4 */
> >         /* --- cacheline 1 boundary (64 bytes) --- */
> >         struct netdev_queue *      dev_queue;            /*    64     8 */
> > -       struct net_rate_estimator * rate_est;            /*    72     8 */
> > -       struct gnet_stats_basic_sync * cpu_bstats;       /*    80     8 */
> > -       struct gnet_stats_queue *  cpu_qstats;           /*    88     8 */
> > +       struct net_rate_estimator  * rate_est;           /*    72     8 */
> > +       struct gnet_stats_basic_sync  * cpu_bstats;      /*    80     8 */
> > +       struct gnet_stats_queue  * cpu_qstats;           /*    88     8 */
> >         int                        pad;                  /*    96     4 */
> >         refcount_t                 refcnt;               /*   100     4 */
> > 
> > @@ -96,8 +96,8 @@
> > 
> >         /* XXX 4 bytes hole, try to pack */
> > 
> > -       long unsigned int          state;                /*   216     8 */
> > -       long unsigned int          state2;               /*   224     8 */
> > +       unsigned long              state;                /*   216     8 */
> > +       unsigned long              state2;               /*   224     8 */
> >         struct Qdisc *             next_sched;           /*   232     8 */
> >         struct sk_buff_head        skb_bad_txq;          /*   240    24 */
> > 
> > @@ -112,7 +112,7 @@
> >         /* XXX 40 bytes hole, try to pack */
> > 
> >         /* --- cacheline 6 boundary (384 bytes) --- */
> > -       long int                   privdata[];           /*   384     0 */
> > +       long                       privdata[];           /*   384     0 */
> > 
> >         /* size: 384, cachelines: 6, members: 28 */
> >         /* sum members: 260, holes: 4, sum holes: 124 */
> > @@ -145,19 +145,19 @@
> >         /* XXX 4 bytes hole, try to pack */
> > 
> >         struct netdev_queue *      (*select_queue)(struct Qdisc *, struct tcmsg *); /*     8     8 */
> > -       int                        (*graft)(struct Qdisc *, long unsigned int, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /*    16     8 */
> > +       int                        (*graft)(struct Qdisc *, unsigned long, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /*    16     8 */
> > -       struct Qdisc *             (*leaf)(struct Qdisc *, long unsigned int); /*    24     8 */
> > +       struct Qdisc *             (*leaf)(struct Qdisc *, unsigned long); /*    24     8 */
> > -       void                       (*qlen_notify)(struct Qdisc *, long unsigned int); /*    32     8 */
> > +       void                       (*qlen_notify)(struct Qdisc *, unsigned long); /*    32     8 */
> > -       long unsigned int          (*find)(struct Qdisc *, u32); /*    40     8 */
> > +       unsigned long              (*find)(struct Qdisc *, u32); /*    40     8 */
> > -       int                        (*change)(struct Qdisc *, u32, u32, struct nlattr * *, long unsigned int *, struct netlink_ext_ack *); /*    48     8 */
> > +       int                        (*change)(struct Qdisc *, u32, u32, struct nlattr * *, unsigned long *, struct netlink_ext_ack *); /*    48     8 */
> > -       int                        (*delete)(struct Qdisc *, long unsigned int, struct netlink_ext_ack *); /*    56     8 */
> > +       int                        (*delete)(struct Qdisc *, unsigned long, struct netlink_ext_ack *); /*    56     8 */
> > 
> > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > index 1e6147a82056c188..1ecc24321bf8f975 100644
> > --- a/dwarves_fprintf.c
> > +++ b/dwarves_fprintf.c
> > @@ -788,8 +788,15 @@ next_type:
> >  			if (n)
> >  				return printed + n;
> >  			if (ptype->tag == DW_TAG_LLVM_annotation) {
> > -				type = ptype;
> > -				goto next_type;
> > +				// FIXME: Just skip this for now, we need to print this annotation
> > +				// to match the original source code.
> > +
> > +				if (ptype->type == 0) {
> > +					printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name);
> > +					break;
> > +				}
> > +
> > +				ptype = cu__type(cu, ptype->type);
> >  			}
> >  			if (ptype->tag == DW_TAG_subroutine_type) {
> >  				printed += ftype__fprintf(tag__ftype(ptype),
> 
> This explains why '*' was missing, but unfortunately it breaks apart
> when there are multiple type tags, e.g.:
> 
> 
>     $ cat tag-test.c
>     #define __t __attribute__((btf_type_tag("t1")))
>     
>     struct foo {
>       int  (__t __t *a)(void);
>     } g;
>     $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
>     struct foo {
>     	int ()(void)   *           a;                    /*     0     8 */
>     
>     	/* size: 8, cachelines: 1, members: 1 */
>     	/* last cacheline: 8 bytes */
>     };
>     $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o
>     struct foo {
>     	int ()(void)   *           a;                    /*     0     8 */
>     
>     	/* size: 8, cachelines: 1, members: 1 */
>     	/* last cacheline: 8 bytes */
>     };
>     
> What I don't understand is why pointer's type is LLVM_annotation.

Well, that is how it is encoded in BTF and then you supported it in:

Author: Eduard Zingerman <eddyz87@gmail.com>
Date:   Wed Mar 15 01:04:14 2023 +0200

    btf_loader: A hack for BTF import of btf_type_tag attributes`


I find it natural, and think that annots thing is a variation on how to
store modifiers for types, see, this DW_TAG_LLVM_annotation is in the
same class as 'restrict', 'const', 'volatile', "atomic", etc

I understand that for encoding _DWARF_ people preferred to make it as a
child DIE to avoid breaking existing DWARF consumers, but in
pahole's dwarf_loader.c we can just make it work like BTF and insert the
btf_type_tag in the chain, just like 'const', etc, no?

pahole wants to print those annotation just like it prints 'const',
'volatile', etc.

> Probably, the cleanest solution would be to make DWARF and BTF loaders
> work in a similar way and attach LLVM_annotation as `annots` field of
> the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's
> in the middle of type chains. I'll work on this.

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-29 15:43               ` Arnaldo Carvalho de Melo
@ 2023-03-29 16:02                 ` Eduard Zingerman
  2023-03-30 11:29                   ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-29 16:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs,
	jose.marchesi, david.faust, alan.maguire

On Wed, 2023-03-29 at 12:43 -0300, Arnaldo Carvalho de Melo wrote:
[...]
> > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > > index 1e6147a82056c188..1ecc24321bf8f975 100644
> > > --- a/dwarves_fprintf.c
> > > +++ b/dwarves_fprintf.c
> > > @@ -788,8 +788,15 @@ next_type:
> > >  			if (n)
> > >  				return printed + n;
> > >  			if (ptype->tag == DW_TAG_LLVM_annotation) {
> > > -				type = ptype;
> > > -				goto next_type;
> > > +				// FIXME: Just skip this for now, we need to print this annotation
> > > +				// to match the original source code.
> > > +
> > > +				if (ptype->type == 0) {
> > > +					printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name);
> > > +					break;
> > > +				}
> > > +
> > > +				ptype = cu__type(cu, ptype->type);
> > >  			}
> > >  			if (ptype->tag == DW_TAG_subroutine_type) {
> > >  				printed += ftype__fprintf(tag__ftype(ptype),
> > 
> > This explains why '*' was missing, but unfortunately it breaks apart
> > when there are multiple type tags, e.g.:
> > 
> > 
> >     $ cat tag-test.c
> >     #define __t __attribute__((btf_type_tag("t1")))
> >     
> >     struct foo {
> >       int  (__t __t *a)(void);
> >     } g;
> >     $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
> >     struct foo {
> >     	int ()(void)   *           a;                    /*     0     8 */
> >     
> >     	/* size: 8, cachelines: 1, members: 1 */
> >     	/* last cacheline: 8 bytes */
> >     };
> >     $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o
> >     struct foo {
> >     	int ()(void)   *           a;                    /*     0     8 */
> >     
> >     	/* size: 8, cachelines: 1, members: 1 */
> >     	/* last cacheline: 8 bytes */
> >     };
> >     
> > What I don't understand is why pointer's type is LLVM_annotation.
> 
> Well, that is how it is encoded in BTF and then you supported it in:
> 
> Author: Eduard Zingerman <eddyz87@gmail.com>
> Date:   Wed Mar 15 01:04:14 2023 +0200
> 
>     btf_loader: A hack for BTF import of btf_type_tag attributes`

To be honest, I was under impression that I add a workaround and the
preferred way is to do what dwarf loader does with
btf_type_tag_ptr_type::annots.
 
> I find it natural, and think that annots thing is a variation on how to
> store modifiers for types, see, this DW_TAG_LLVM_annotation is in the
> same class as 'restrict', 'const', 'volatile', "atomic", etc
> 
> I understand that for encoding _DWARF_ people preferred to make it as a
> child DIE to avoid breaking existing DWARF consumers, but in
> pahole's dwarf_loader.c we can just make it work like BTF and insert the
> btf_type_tag in the chain, just like 'const', etc, no?
> 
> pahole wants to print those annotation just like it prints 'const',
> 'volatile', etc.

Actually, if reflecting physical structure of the DWARF is not a goal,
forgoing "annots" fields altogether and treating type tags as derived
types should make support for btf:type_tag (the v2 version) simpler.

Then, getting back to the current issue, I need to add
skip_llvm_annotations function with a loop inside, right?

> > Probably, the cleanest solution would be to make DWARF and BTF loaders
> > work in a similar way and attach LLVM_annotation as `annots` field of
> > the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's
> > in the middle of type chains. I'll work on this.
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-29 16:02                 ` Eduard Zingerman
@ 2023-03-30 11:29                   ` Arnaldo Carvalho de Melo
  2023-03-30 12:34                     ` Eduard Zingerman
  0 siblings, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-30 11:29 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Arnaldo Carvalho de Melo, dwarves, bpf, kernel-team, ast, daniel,
	andrii, yhs, jose.marchesi, david.faust, alan.maguire

Em Wed, Mar 29, 2023 at 07:02:45PM +0300, Eduard Zingerman escreveu:
> On Wed, 2023-03-29 at 12:43 -0300, Arnaldo Carvalho de Melo wrote:
> [...]
> > > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > > > index 1e6147a82056c188..1ecc24321bf8f975 100644
> > > > --- a/dwarves_fprintf.c
> > > > +++ b/dwarves_fprintf.c
> > > > @@ -788,8 +788,15 @@ next_type:
> > > >  			if (n)
> > > >  				return printed + n;
> > > >  			if (ptype->tag == DW_TAG_LLVM_annotation) {
> > > > -				type = ptype;
> > > > -				goto next_type;
> > > > +				// FIXME: Just skip this for now, we need to print this annotation
> > > > +				// to match the original source code.
> > > > +
> > > > +				if (ptype->type == 0) {
> > > > +					printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name);
> > > > +					break;
> > > > +				}
> > > > +
> > > > +				ptype = cu__type(cu, ptype->type);
> > > >  			}
> > > >  			if (ptype->tag == DW_TAG_subroutine_type) {
> > > >  				printed += ftype__fprintf(tag__ftype(ptype),
> > > 
> > > This explains why '*' was missing, but unfortunately it breaks apart
> > > when there are multiple type tags, e.g.:
> > > 
> > > 
> > >     $ cat tag-test.c
> > >     #define __t __attribute__((btf_type_tag("t1")))
> > >     
> > >     struct foo {
> > >       int  (__t __t *a)(void);
> > >     } g;
> > >     $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
> > >     struct foo {
> > >     	int ()(void)   *           a;                    /*     0     8 */
> > >     
> > >     	/* size: 8, cachelines: 1, members: 1 */
> > >     	/* last cacheline: 8 bytes */
> > >     };
> > >     $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o
> > >     struct foo {
> > >     	int ()(void)   *           a;                    /*     0     8 */
> > >     
> > >     	/* size: 8, cachelines: 1, members: 1 */
> > >     	/* last cacheline: 8 bytes */
> > >     };
> > >     
> > > What I don't understand is why pointer's type is LLVM_annotation.
> > 
> > Well, that is how it is encoded in BTF and then you supported it in:
> > 
> > Author: Eduard Zingerman <eddyz87@gmail.com>
> > Date:   Wed Mar 15 01:04:14 2023 +0200
> > 
> >     btf_loader: A hack for BTF import of btf_type_tag attributes`
> 
> To be honest, I was under impression that I add a workaround and the
> preferred way is to do what dwarf loader does with
> btf_type_tag_ptr_type::annots.
>  
> > I find it natural, and think that annots thing is a variation on how to
> > store modifiers for types, see, this DW_TAG_LLVM_annotation is in the
> > same class as 'restrict', 'const', 'volatile', "atomic", etc
> > 
> > I understand that for encoding _DWARF_ people preferred to make it as a
> > child DIE to avoid breaking existing DWARF consumers, but in
> > pahole's dwarf_loader.c we can just make it work like BTF and insert the
> > btf_type_tag in the chain, just like 'const', etc, no?
> > 
> > pahole wants to print those annotation just like it prints 'const',
> > 'volatile', etc.
> 
> Actually, if reflecting physical structure of the DWARF is not a goal,

Well reflecting the physical structure of DWARF _pre_
DW_TAG_llvm_annotation was the goal, but now, since this was done
differently of DW_TAG_const_type, DW_TAG_pointer_type, etc, as one link
in the chain, to avoid breaking existing DWARF consumers, we ended up
with this annot thing, but the internal representation in pahole can
continue being as a chain, as BTF does, right?

> forgoing "annots" fields altogether and treating type tags as derived
> types should make support for btf:type_tag (the v2 version) simpler.

I think it should simplify as we're used to these chains.
 
> Then, getting back to the current issue, I need to add
> skip_llvm_annotations function with a loop inside, right?

You can, to remove them completely and its like they don't exist for
dwarf_fprintf.c, but what I think should be done is to actually print
them, to reconstruct the original source code.

You can start removing them and we can work later on properly printing
it, so that we can get 1.25 out of the door as there are multiple
requests for it to be released to solve other problems with using 1.24.

- Arnaldo
 
> > > Probably, the cleanest solution would be to make DWARF and BTF loaders
> > > work in a similar way and attach LLVM_annotation as `annots` field of
> > > the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's
> > > in the middle of type chains. I'll work on this.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
  2023-03-30 11:29                   ` Arnaldo Carvalho de Melo
@ 2023-03-30 12:34                     ` Eduard Zingerman
  0 siblings, 0 replies; 21+ messages in thread
From: Eduard Zingerman @ 2023-03-30 12:34 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs,
	jose.marchesi, david.faust, alan.maguire

On Thu, 2023-03-30 at 08:29 -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 29, 2023 at 07:02:45PM +0300, Eduard Zingerman escreveu:
> > On Wed, 2023-03-29 at 12:43 -0300, Arnaldo Carvalho de Melo wrote:
> > [...]
> > > > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > > > > index 1e6147a82056c188..1ecc24321bf8f975 100644
> > > > > --- a/dwarves_fprintf.c
> > > > > +++ b/dwarves_fprintf.c
> > > > > @@ -788,8 +788,15 @@ next_type:
> > > > >  			if (n)
> > > > >  				return printed + n;
> > > > >  			if (ptype->tag == DW_TAG_LLVM_annotation) {
> > > > > -				type = ptype;
> > > > > -				goto next_type;
> > > > > +				// FIXME: Just skip this for now, we need to print this annotation
> > > > > +				// to match the original source code.
> > > > > +
> > > > > +				if (ptype->type == 0) {
> > > > > +					printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name);
> > > > > +					break;
> > > > > +				}
> > > > > +
> > > > > +				ptype = cu__type(cu, ptype->type);
> > > > >  			}
> > > > >  			if (ptype->tag == DW_TAG_subroutine_type) {
> > > > >  				printed += ftype__fprintf(tag__ftype(ptype),
> > > > 
> > > > This explains why '*' was missing, but unfortunately it breaks apart
> > > > when there are multiple type tags, e.g.:
> > > > 
> > > > 
> > > >     $ cat tag-test.c
> > > >     #define __t __attribute__((btf_type_tag("t1")))
> > > >     
> > > >     struct foo {
> > > >       int  (__t __t *a)(void);
> > > >     } g;
> > > >     $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
> > > >     struct foo {
> > > >     	int ()(void)   *           a;                    /*     0     8 */
> > > >     
> > > >     	/* size: 8, cachelines: 1, members: 1 */
> > > >     	/* last cacheline: 8 bytes */
> > > >     };
> > > >     $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o
> > > >     struct foo {
> > > >     	int ()(void)   *           a;                    /*     0     8 */
> > > >     
> > > >     	/* size: 8, cachelines: 1, members: 1 */
> > > >     	/* last cacheline: 8 bytes */
> > > >     };
> > > >     
> > > > What I don't understand is why pointer's type is LLVM_annotation.
> > > 
> > > Well, that is how it is encoded in BTF and then you supported it in:
> > > 
> > > Author: Eduard Zingerman <eddyz87@gmail.com>
> > > Date:   Wed Mar 15 01:04:14 2023 +0200
> > > 
> > >     btf_loader: A hack for BTF import of btf_type_tag attributes`
> > 
> > To be honest, I was under impression that I add a workaround and the
> > preferred way is to do what dwarf loader does with
> > btf_type_tag_ptr_type::annots.
> >  
> > > I find it natural, and think that annots thing is a variation on how to
> > > store modifiers for types, see, this DW_TAG_LLVM_annotation is in the
> > > same class as 'restrict', 'const', 'volatile', "atomic", etc
> > > 
> > > I understand that for encoding _DWARF_ people preferred to make it as a
> > > child DIE to avoid breaking existing DWARF consumers, but in
> > > pahole's dwarf_loader.c we can just make it work like BTF and insert the
> > > btf_type_tag in the chain, just like 'const', etc, no?
> > > 
> > > pahole wants to print those annotation just like it prints 'const',
> > > 'volatile', etc.
> > 
> > Actually, if reflecting physical structure of the DWARF is not a goal,
> 
> Well reflecting the physical structure of DWARF _pre_
> DW_TAG_llvm_annotation was the goal, but now, since this was done
> differently of DW_TAG_const_type, DW_TAG_pointer_type, etc, as one link
> in the chain, to avoid breaking existing DWARF consumers, we ended up
> with this annot thing, but the internal representation in pahole can
> continue being as a chain, as BTF does, right?
 
Ok, I'll rework the patch-set and we'll see, my expectation is that
working with BTF style structure would be simpler.

> > forgoing "annots" fields altogether and treating type tags as derived
> > types should make support for btf:type_tag (the v2 version) simpler.
> 
> I think it should simplify as we're used to these chains.
>  
> > Then, getting back to the current issue, I need to add
> > skip_llvm_annotations function with a loop inside, right?
> 
> You can, to remove them completely and its like they don't exist for
> dwarf_fprintf.c, but what I think should be done is to actually print
> them, to reconstruct the original source code.
> 
> You can start removing them and we can work later on properly printing
> it, so that we can get 1.25 out of the door as there are multiple
> requests for it to be released to solve other problems with using 1.24.

Understood, I'll send an update that ignores type tags properly today,
after some testing, and add printing as a follow-up.

> 
> - Arnaldo
>  
> > > > Probably, the cleanest solution would be to make DWARF and BTF loaders
> > > > work in a similar way and attach LLVM_annotation as `annots` field of
> > > > the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's
> > > > in the middle of type chains. I'll work on this.


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2023-03-30 12:34 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-14 23:04 [PATCH dwarves v2 0/5] Support for new btf_type_tag encoding Eduard Zingerman
2023-03-14 23:04 ` [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute Eduard Zingerman
2023-03-27 11:46   ` Arnaldo Carvalho de Melo
2023-03-27 12:10     ` Eduard Zingerman
2023-03-27 12:55       ` Arnaldo Carvalho de Melo
2023-03-28 12:40       ` Arnaldo Carvalho de Melo
2023-03-28 13:40         ` Eduard Zingerman
2023-03-28 13:59   ` Arnaldo Carvalho de Melo
2023-03-28 14:08     ` Eduard Zingerman
2023-03-28 15:26       ` Arnaldo Carvalho de Melo
2023-03-28 15:30         ` Eduard Zingerman
2023-03-28 21:17           ` Arnaldo Carvalho de Melo
2023-03-29 15:36             ` Eduard Zingerman
2023-03-29 15:43               ` Arnaldo Carvalho de Melo
2023-03-29 16:02                 ` Eduard Zingerman
2023-03-30 11:29                   ` Arnaldo Carvalho de Melo
2023-03-30 12:34                     ` Eduard Zingerman
2023-03-14 23:04 ` [PATCH dwarves v2 2/5] btf_loader: A hack for BTF import of btf_type_tag attributes Eduard Zingerman
2023-03-14 23:04 ` [PATCH dwarves v2 3/5] dwarf_loader: Consolidate llvm_annotation and btf_type_tag_type Eduard Zingerman
2023-03-14 23:04 ` [PATCH dwarves v2 4/5] dwarf_loader: Track unspecified types in a separate list Eduard Zingerman
2023-03-14 23:04 ` [PATCH dwarves v2 5/5] dwarf_loader: Support for btf:type_tag Eduard Zingerman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).