On Tue, May 6, 2025 at 9:34 PM David Faust <david.fa...@oracle.com> wrote:
>
>
>
> On 5/6/25 01:24, Richard Biener wrote:
> > On Mon, May 5, 2025 at 10:40 PM David Faust <david.fa...@oracle.com> wrote:
> >>
> >>
> >>
> >> On 5/2/25 01:26, Richard Biener wrote:
> >>> On Wed, Apr 30, 2025 at 7:26 PM David Faust <david.fa...@oracle.com>
> >>> wrote:
> >>>>
> >>>> The btf_decl_tag and btf_type_tag attributes provide a means to annotate
> >>>> declarations and types respectively with arbitrary user provided
> >>>> strings. These strings are recorded in debug information for
> >>>> post-compilation uses, and despite the name they are meant to be
> >>>> recorded in DWARF as well as BTF. New DWARF extensions
> >>>> DW_TAG_GNU_annotation and DW_AT_GNU_annotation are used to represent
> >>>> these user annotations in DWARF.
> >>>>
> >>>> This patch introduces the new DWARF extension DIE and attribute, and
> >>>> generates them as necessary to represent user annotations from
> >>>> btf_decl_tag and btf_type_tag.
> >>>>
> >>>> The format of the new DIE is as follows:
> >>>>
> >>>> DW_TAG_GNU_annotation
> >>>> DW_AT_name: "btf_decl_tag" or "btf_type_tag"
> >>>> DW_AT_const_value: <arbitrary user-supplied string>
> >>>> DW_AT_GNU_annotation: <reference to another TAG_GNU_annotation
> >>>> DIE>
> >>>>
> >>>> DW_AT_GNU_annotation is a new attribute extension used to refer to these
> >>>> new annotation DIEs. If non-null in any given declaration or type DIE,
> >>>> it is a reference to a DW_TAG_GNU_annotation DIE holding an annotation
> >>>> for that declaration or type. In addition, the DW_TAG_GNU_annotation
> >>>> DIEs may also have a non-null DW_AT_GNU_annotation, referring to another
> >>>> annotation DIE. This allows chains of annotation DIEs to be formed,
> >>>> such as in the case where a single declaration has multiple instances of
> >>>> btf_decl_tag with different string annotations.
> >>>>
> >>>> gcc/
> >>>> * dwarf2out.cc (struct annotation_node, struct
> >>>> annotation_node_hasher)
> >>>> (btf_tag_htab): New ancillary structures and hash table.
> >>>> (annotation_node_hasher::hash, annotation_node_hasher::equal):
> >>>> New.
> >>>> (hash_btf_tag, gen_btf_tag_dies, gen_btf_type_tag_dies)
> >>>> (maybe_gen_btf_type_tag_dies, gen_btf_decl_tag_dies): New
> >>>> functions.
> >>>> (modified_type_die): Handle btf_type_tag attribute.
> >>>> (gen_array_type_die): Call maybe_gen_btf_type_tags for the type.
> >>>> (gen_formal_parameter_die): Call gen_btf_decl_tags for the
> >>>> parameter.
> >>>> (gen_decl_die): Call gen_btf_decl_tags for the decl.
> >>>> (gen_tagged_type_die): Call maybe_gen_btf_type_tag_dies for the
> >>>> type.
> >>>> (dwarf2out_early_finish): Empty btf_tag_htab hash table.
> >>>> (dwarf2out_cc_finalize): Delete btf_tag_htab hash table.
> >>>>
> >>>> include/
> >>>> * dwarf2.def (DW_TAG_GNU_annotation): New DWARF extension.
> >>>> (DW_AT_GNU_annotation): Likewise.
> >>>>
> >>>> gcc/testsuite/
> >>>> * gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-1.c: New test.
> >>>> * gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-2.c: New test.
> >>>> * gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-3.c: New test.
> >>>> * gcc.dg/debug/dwarf2/dwarf-btf-type-tag-1.c: New test.
> >>>> * gcc.dg/debug/dwarf2/dwarf-btf-type-tag-2.c: New test.
> >>>> * gcc.dg/debug/dwarf2/dwarf-btf-type-tag-3.c: New test.
> >>>> * gcc.dg/debug/dwarf2/dwarf-btf-type-tag-4.c: New test.
> >>>> * gcc.dg/debug/dwarf2/dwarf-btf-type-tag-5.c: New test.
> >>>> ---
> >>>> gcc/dwarf2out.cc | 270 +++++++++++++++++-
> >>>> .../debug/dwarf2/dwarf-btf-decl-tag-1.c | 11 +
> >>>> .../debug/dwarf2/dwarf-btf-decl-tag-2.c | 25 ++
> >>>> .../debug/dwarf2/dwarf-btf-decl-tag-3.c | 21 ++
> >>>> .../debug/dwarf2/dwarf-btf-type-tag-1.c | 10 +
> >>>> .../debug/dwarf2/dwarf-btf-type-tag-2.c | 31 ++
> >>>> .../debug/dwarf2/dwarf-btf-type-tag-3.c | 15 +
> >>>> .../debug/dwarf2/dwarf-btf-type-tag-4.c | 33 +++
> >>>> .../debug/dwarf2/dwarf-btf-type-tag-5.c | 10 +
> >>>> include/dwarf2.def | 4 +
> >>>> 10 files changed, 426 insertions(+), 4 deletions(-)
> >>>> create mode 100644
> >>>> gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-1.c
> >>>> create mode 100644
> >>>> gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-2.c
> >>>> create mode 100644
> >>>> gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-3.c
> >>>> create mode 100644
> >>>> gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-1.c
> >>>> create mode 100644
> >>>> gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-2.c
> >>>> create mode 100644
> >>>> gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-3.c
> >>>> create mode 100644
> >>>> gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-4.c
> >>>> create mode 100644
> >>>> gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-5.c
> >>>>
> >>>> diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
> >>>> index 34ffeed86ff..1ec3b24e773 100644
> >>>> --- a/gcc/dwarf2out.cc
> >>>> +++ b/gcc/dwarf2out.cc
> >>>> @@ -3696,6 +3696,32 @@ static bool frame_pointer_fb_offset_valid;
> >>>>
> >>>> static vec<dw_die_ref> base_types;
> >>>>
> >>>> +/* A cached btf_type_tag or btf_decl_tag user annotation. */
> >>>> +struct GTY ((for_user)) annotation_node
> >>>> +{
> >>>> + const char *name;
> >>>> + const char *value;
> >>>> + hashval_t hash;
> >>>> + dw_die_ref die;
> >>>> + struct annotation_node *next;
> >>>> +};
> >>>> +
> >>>> +struct annotation_node_hasher : ggc_ptr_hash<annotation_node>
> >>>> +{
> >>>> + typedef const struct annotation_node *compare_type;
> >>>> +
> >>>> + static hashval_t hash (struct annotation_node *);
> >>>> + static bool equal (const struct annotation_node *,
> >>>> + const struct annotation_node *);
> >>>> +};
> >>>> +
> >>>> +/* A hash table of tag annotation nodes for btf_type_tag and
> >>>> btf_decl_tag C
> >>>> + attributes. DIEs for these user annotations may be reused if they
> >>>> are
> >>>> + structurally equivalent; this hash table is used to ensure the DIEs
> >>>> are
> >>>> + reused wherever possible. */
> >>>> +static GTY (()) hash_table<annotation_node_hasher> *btf_tag_htab;
> >>>> +
> >>>> +
> >>>> /* Flags to represent a set of attribute classes for attributes that
> >>>> represent
> >>>> a scalar value (bounds, pointers, ...). */
> >>>> enum dw_scalar_form
> >>>> @@ -13659,6 +13685,180 @@ long_double_as_float128 (tree type)
> >>>> return NULL_TREE;
> >>>> }
> >>>>
> >>>> +
> >>>> +hashval_t
> >>>> +annotation_node_hasher::hash (struct annotation_node *node)
> >>>> +{
> >>>> + return node->hash;
> >>>> +}
> >>>> +
> >>>> +bool
> >>>> +annotation_node_hasher::equal (const struct annotation_node *node1,
> >>>> + const struct annotation_node *node2)
> >>>> +{
> >>>> + return (node1->hash == node2->hash);
> >>>
> >>> never any hash collision? I'd have expected name/value to be compared
> >>> here,
> >>> and also 'next' (the "chain")?
> >>
> >> Thanks, you're right. name/value/chain go into the hash but I need to
> >> check for collisions here.
> >>
> >>>
> >>> Are we ever able to share partial chains this way? LTO streaming does
> >>> for example "share" list tails. I think attribute list generation
> >>> does not perform
> >>> any attempt to share.
> >>
> >> Yes, we are able to share common list tails of the attribute. But it does
> >> depend on the ordering of the attribute list that we get.
> >>
> >> For example, if __tagN are all btf_type_tag("tagN") attributes:
> >>
> >> int * __tag1 __tag2 __tag3 foo;
> >> char * __tag1 __tag2 __tag4 bar;
> >>
> >> In this case the sub-chain for (__tag1 __tag2) is shared, because the
> >> attribute lists for the two types have them in the same order
> >> (the actual tree_list nodes are not shared despite same contents).
> >> Then __tag3 or __tag4 can be tacked on to the head of that common tail,
> >> and the result is only one TAG_annotation DIE for each tag is produced.
> >>
> >> But if we add:
> >>
> >> float __tag2 __tag1 __tag3 baz;
> >>
> >> Currently this will not share the above sub-chain, because the attribute
> >> list for this type has a different ordering than above.
> >>
> >> This could be improved by enforcing our own ordering when processing the
> >> attribute list for tags, or maybe better by enforcing an ordering when
> >> the attribute list is constructed. That would require looking also at
> >> the attribute value, which I think we would need to do anyway as a step
> >> to making attribute lists sharable. I may look into that in the future,
> >> but IMO a little out of scope of this series.
> >>
> >>>
> >>>> +}
> >>>> +
> >>>> +/* Return an appropriate entry in the btf tag hash table for a given
> >>>> btf tag.
> >>>> + If a structurally equivalent tag (one with the same name, value, and
> >>>> + subsequent chain of further tags) has already been processed, then
> >>>> the
> >>>> + existing entry for that tag is returned and should be reused.
> >>>> + Otherwise, a new entry is added to the hash table and returned. */
> >>>> +
> >>>> +static struct annotation_node *
> >>>> +hash_btf_tag (tree attr)
> >>>> +{
> >>>> + if (attr == NULL_TREE || TREE_CODE (attr) != TREE_LIST)
> >>>> + return NULL;
> >>>> +
> >>>> + if (!btf_tag_htab)
> >>>> + btf_tag_htab = hash_table<annotation_node_hasher>::create_ggc (10);
> >>>> +
> >>>> + const char * name = IDENTIFIER_POINTER (get_attribute_name (attr));
> >>>> + const char * value = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE
> >>>> (attr)));
> >>>> + tree chain = lookup_attribute (name, TREE_CHAIN (attr));
> >>>> +
> >>>> + /* Hash for one tag depends on hash of next tag in the chain, because
> >>>> + the chain is part of structural equivalence. */
> >>>> + struct annotation_node *chain_node = hash_btf_tag (chain);
> >>>> + gcc_checking_assert (chain == NULL_TREE || chain_node != NULL);
> >>>> +
> >>>> + /* Skip any non-btf-tag attributes that might be in the chain. */
> >>>> + if (strcmp (name, "btf_type_tag") != 0 && strcmp (name,
> >>>> "btf_decl_tag") != 0)
> >>>> + return chain_node;
> >>>> +
> >>>> + /* Hash for a given tag is determined by the name, value, and chain of
> >>>> + further tags. */
> >>>> + inchash::hash h;
> >>>> + h.merge_hash (htab_hash_string (name));
> >>>> + h.merge_hash (htab_hash_string (value));
> >>>> + h.merge_hash (chain_node ? chain_node->hash : 0);
> >>>> +
> >>>> + struct annotation_node node;
> >>>> + node.name = name;
> >>>> + node.value = value;
> >>>> + node.hash = h.end ();
> >>>> + node.next = chain_node;
> >>>> +
> >>>> + struct annotation_node **slot = btf_tag_htab->find_slot (&node,
> >>>> INSERT);
> >>>> + if (*slot == NULL)
> >>>> + {
> >>>> + /* Create new htab entry for this annotation. */
> >>>> + struct annotation_node *new_slot
> >>>> + = ggc_cleared_alloc<struct annotation_node> ();
> >>>> + new_slot->name = name;
> >>>> + new_slot->value = value;
> >>>> + new_slot->hash = node.hash;
> >>>> + new_slot->next = chain_node;
> >>>> +
> >>>> + *slot = new_slot;
> >>>> + return new_slot;
> >>>> + }
> >>>> + else
> >>>> + {
> >>>> + /* This node is already in the hash table. */
> >>>> + return *slot;
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +/* Generate (or reuse) DW_TAG_annotation DIEs representing the
> >>>> btf_type_tag or
> >>>> + btf_decl_tag user annotations in ATTR, and update DIE to refer to
> >>>> them
> >>>> + via DW_AT_annotation. If there are multiple type_tag or decl_tag
> >>>> + annotations in ATTR, they are all processed recursively by this
> >>>> function
> >>>> + to build a chain of annotation DIEs.
> >>>> + Return the first annotation DIE in the created (or reused) chain. */
> >>>> +
> >>>> +static dw_die_ref
> >>>> +gen_btf_tag_dies (tree attr, dw_die_ref die, dw_die_ref context_die)
> >>>> +{
> >>>> + if (attr == NULL_TREE)
> >>>> + return die;
> >>>> +
> >>>> + while (dw_get_die_tag (context_die) != DW_TAG_compile_unit)
> >>>> + context_die = context_die->die_parent;
> >>>> +
> >>>> + const char * name = IDENTIFIER_POINTER (get_attribute_name (attr));
> >>>> + const char * value = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE
> >>>> (attr)));
> >>>> +
> >>>> + dw_die_ref tag_die, prev = NULL;
> >>>> +
> >>>> + /* Multiple annotations on the same item form a singly-linked list of
> >>>> + annotation DIEs; generate recursively backward from the end so we
> >>>> can
> >>>> + chain each created DIE to the next, which has already been
> >>>> created. */
> >>>> + tree rest = lookup_attribute (name, TREE_CHAIN (attr));
> >>>> + if (rest)
> >>>> + prev = gen_btf_tag_dies (rest, NULL, context_die);
> >>>> +
> >>>> + /* Calculate a hash value for the tag based on its structure, find the
> >>>> + existing entry for it (if any) in the hash table, or create a new
> >>>> entry
> >>>> + which can be reused by structurally-equivalent tags. */
> >>>> + struct annotation_node *entry = hash_btf_tag (attr);
> >>>> + if (!entry)
> >>>> + return die;
> >>>> +
> >>>> + /* If the node already has an associated DIE, reuse it.
> >>>> + Otherwise, create the new annotation DIE, and associate it with
> >>>> + the hash table entry for future reuse. Any structurally-equivalent
> >>>> + tag we process later will find and share the same DIE. */
> >>>> + if (entry->die)
> >>>> + tag_die = entry->die;
> >>>> + else
> >>>> + {
> >>>> + tag_die = new_die (DW_TAG_GNU_annotation, context_die, NULL);
> >>>> + add_name_attribute (tag_die, name);
> >>>> + add_AT_string (tag_die, DW_AT_const_value, value);
> >>>> + if (prev)
> >>>> + add_AT_die_ref (tag_die, DW_AT_GNU_annotation, prev);
> >>>
> >>> So I can see this saves DWARF space compared to adding multiple
> >>> DW_AT_GNU_annotation to the annotated object. It does feel a bit
> >>> un-DWARFish, I suppose using DIE children (with the same tag?)
> >>> would have been more DWARFish here. That would of course
> >>> remove the ability to share sub-lists (but as said, I'm unsure this
> >>> works now). So for a three-item list you'd have
> >>>
> >>> DW_TAG_GNU_annotation
> >>> name
> >>> value
> >>> DW_TAG_GNU_annotation (first child)
> >>> name
> >>> value
> >>> DW_TAG_GNU_annotation (second child)
> >>> name
> >>> value
> >>>
> >>> or alternatively, make the outer one "empty" (a container)
> >>> and add a third child with the above outers name/value.
> >>>
> >>> It's a bit of bike-shedding of course.
> >>>
> >>
> >> Hmm, interesting. This is a little similar to what clang does now,
> >> except that they attach the annotation DIEs as children directly to
> >> the type DIE, which eliminates sharing altogether and was the main
> >> driver to look into a different format.
> >>
> >> So iiuc this is a sort of hybrid, to use the new AT_annotation
> >> along with the simpler child-DIE approach. That way we can still
> >> share since all types with the same set of tags can use AT_annotation
> >> to point to the same TAG_annotation DIE.
> >>
> >> I agree this is probably a little more simple and a little more
> >> DWARFish. Not sure whether or not the tradeoff with sharing sub-chains
> >> is worth it.
> >>
> >> The way type_tag is currently used in the kernel, the common case is
> >> that there is a small set of distinct values (e.g. "user", "kernel",
> >> "rcu", a few others) and there are many occurrences types which have
> >> one or maybe two of these tags. I do think type_tags are an interesting
> >> concept which have several potential applications beyond the kernel
> >> use case though.
> >>
> >>>> +
> >>>> + entry->die = tag_die;
> >>>> + }
> >>>> +
> >>>> + if (die)
> >>>> + add_AT_die_ref (die, DW_AT_GNU_annotation, tag_die);
> >>>> +
> >>>> + return tag_die;
> >>>> +}
> >>>> +
> >>>> +static void
> >>>> +gen_btf_type_tag_dies (tree t, tree attr, dw_die_ref target,
> >>>> + dw_die_ref context_die)
> >>>> +{
> >>>> + if (t == NULL_TREE || !TYPE_P (t) || !target)
> >>>> + return;
> >>>> +
> >>>> + gen_btf_tag_dies (attr, target, context_die);
> >>>> +}
> >>>> +
> >>>> +static void
> >>>> +maybe_gen_btf_type_tag_dies (tree t, dw_die_ref die, dw_die_ref
> >>>> context_die)
> >>>> +{
> >>>> + tree tags = lookup_attribute ("btf_type_tag", TYPE_ATTRIBUTES (t));
> >>>> + if (tags)
> >>>> + {
> >>>> + TYPE_ATTRIBUTES (t)
> >>>> + = remove_attribute ("btf_type_tag", TYPE_ATTRIBUTES (t));
> >>>> + gen_btf_type_tag_dies (t, tags, die, context_die);
> >>>> + }
> >>>> +}
> >>>> +
> >>>
> >>> Most of the functions miss a function-level comment.
> >>
> >> Oops, thanks.
> >>
> >>>
> >>>> +static void
> >>>> +gen_btf_decl_tag_dies (tree t, dw_die_ref target, dw_die_ref
> >>>> context_die)
> >>>> +{
> >>>> + if (t == NULL_TREE || !DECL_P (t) || !target)
> >>>> + return;
> >>>> +
> >>>> + tree attr = lookup_attribute ("btf_decl_tag", DECL_ATTRIBUTES (t));
> >>>> + if (attr == NULL_TREE)
> >>>> + return;
> >>>> +
> >>>> + gen_btf_tag_dies (attr, target, context_die);
> >>>> +
> >>>> + /* Strip the decl tag attribute once we have created the annotation
> >>>> DIEs
> >>>> + to avoid attempting process it multiple times. */
> >>>> + DECL_ATTRIBUTES (t)
> >>>> + = remove_attribute ("btf_decl_tag", DECL_ATTRIBUTES (t));
> >>>
> >>> Hmm, you should get a type/decl DIE created only once, so why would you
> >>> run into this multiple times?
> >>
> >> For decls, I see that we go through gen_decl_die twice for global
> >> variables;
> >> first from c_parser_declaration_or_fndef via finish_decl, and then again
> >> later from c_write_global_declarations_1.
> >>
> >> For types, it is due to the recursion in modified_type_die.
> >>
> >>>
> >>>> +}
> >>>> +
> >>>> /* Given a pointer to an arbitrary ..._TYPE tree node, return a
> >>>> debugging
> >>>> entry that chains the modifiers specified by CV_QUALS in front of the
> >>>> given type. REVERSE is true if the type is to be interpreted in the
> >>>> @@ -13783,10 +13983,44 @@ modified_type_die (tree type, int cv_quals,
> >>>> bool reverse,
> >>>> dquals &= cv_qual_mask;
> >>>> if ((dquals & ~cv_quals) != TYPE_UNQUALIFIED
> >>>> || (cv_quals == dquals && DECL_ORIGINAL_TYPE (name) !=
> >>>> type))
> >>>> - /* cv-unqualified version of named type. Just use
> >>>> - the unnamed type to which it refers. */
> >>>> - return modified_type_die (DECL_ORIGINAL_TYPE (name),
> >>>> cv_quals,
> >>>> - reverse, context_die);
> >>>> + {
> >>>> + if (lookup_attribute ("btf_type_tag", TYPE_ATTRIBUTES
> >>>> (type)))
> >>>> + {
> >>>> + /* Use of a typedef with additional btf_type_tags.
> >>>> Create
> >>>> + a distinct typedef DIE for this version of the
> >>>> named type
> >>>> + so that the btf_type_tag annotations may be
> >>>> attached to
> >>>> + it without affecting other users of the plain
> >>>> typedef. */
> >>>> + tree tags = lookup_attribute ("btf_type_tag",
> >>>> + TYPE_ATTRIBUTES (type));
> >>>
> >>> you did this in the if () check already. Note I think this is only
> >>> executed
> >>> when the type was qualified or typedef-ed?
> >>
> >> OK, will fix/reuse the lookup_attribute in the if() here and elsewhere.
> >>
> >> Yes, this block is for dealing with type_tag on typedefs, since it is
> >> possible to add a type_tag at the use of a typedef'd type like
> >>
> >> typedef const int __tag1 my_int;
> >> my_int __tag2 a;
> >> my_int __tag3 b;
> >> my_int c;
> >>
> >> We need to construct a few distinct typedef DIEs here so that the different
> >> tags can be attached to their respective versions without disrupting other
> >> users of the underlying typedef.
> >>
> >>>
> >>>> + tree dtags = lookup_attribute ("btf_type_tag",
> >>>> + TYPE_ATTRIBUTES
> >>>> (dtype));
> >>>> +
> >>>> + /* Remove type tags before the recursive call to avoid
> >>>> + processing the same attribute multiple times. */
> >>>> + TYPE_ATTRIBUTES (type)
> >>>> + = remove_attribute ("btf_type_tag", TYPE_ATTRIBUTES
> >>>> (type));
> >>>> +
> >>>> + dw_die_ref mod_die = modified_type_die (dtype,
> >>>> cv_quals,
> >>>> + reverse,
> >>>> context_die);
> >>>> +
> >>>> + /* Create a new typedef DIE since the btf_type_tag-ed
> >>>> use of
> >>>> + the typedef has really created a distinct type. */
> >>>> + if (!attribute_list_equal (tags, dtags))
> >>>> + {
> >>>> + mod_die = clone_die (mod_die);
> >>>> + add_child_die (comp_unit_die (), mod_die);
> >>>> + if (!lookup_type_die (type))
> >>>> + equate_type_number_to_die (type, mod_die);
> >>>> + }
> >>>> +
> >>>> + gen_btf_type_tag_dies (type, tags, mod_die,
> >>>> context_die);
> >>>> + return mod_die;
> >>>> + }
> >>>> + /* cv-unqualified version of named type. Just use
> >>>> + the unnamed type to which it refers. */
> >>>> + return modified_type_die (DECL_ORIGINAL_TYPE (name),
> >>>> cv_quals,
> >>>> + reverse, context_die);
> >>>> + }
> >>>> /* Else cv-qualified version of named type; fall through. */
> >>>> }
> >>>> }
> >>>> @@ -13887,6 +14121,18 @@ modified_type_die (tree type, int cv_quals,
> >>>> bool reverse,
> >>>> first_quals |= dwarf_qual_info[i].q;
> >>>> }
> >>>> }
> >>>> + else if (lookup_attribute ("btf_type_tag", TYPE_ATTRIBUTES (type)))
> >>>> + {
> >>>> + /* Remove type tags before the recursive call to avoid processing
> >>>> the
> >>>> + same attribute multiple times. */
> >>>> + tree attr = lookup_attribute ("btf_type_tag", TYPE_ATTRIBUTES
> >>>> (type));
> >>>
> >>> twice again - if (tree attr = lookup_...) is a thing.
> >>>
> >>>> + TYPE_ATTRIBUTES (type) = remove_attribute ("btf_type_tag",
> >>>> + TYPE_ATTRIBUTES (type));
> >>>> + dw_die_ref mod_die = modified_type_die (type, cv_quals, reverse,
> >>>> + context_die);
> >>>
> >>> While recusing might look "convenient" here I think given
> >>> modified_type_die's
> >>> complexity a more obvious handling should be prefered ;)
> >>
> >> Yes.. I have had quite some trouble with modified_type_die and doing the
> >> right
> >> thing in all various constructs with typedefs, cv-quals, and now type_tags
> >> as
> >> another axis of complexity.
> >>
> >> Did you a particular approach in mind?
> >> I have tried a few non-recursive alternatives, but I didn't land on
> >> anything
> >> that was actually easier to reason about than the recursive call, without
> >> duplicating a lot of logic already written elsewhere.
> >>
> >> It seems to me that in general we need the recursion, following the same
> >> approach as is done for cv-qual'd types above: process the qualifier(s)
> >> (here type_tags) and then recurse with a less-qualified type.
> >>
> >>>
> >>> I think you want the tags to work like qualifiers, but I'm not sure
> >>> how this works
> >>> with your patch. Consider
> >>>
> >>> struct S { int i ; };
> >>>
> >>> struct S __attribute__((btf_type_tag("foo"))) a;
> >>> const struct S b;
> >>> struct S c;
> >>> const struct S __attribute__((btf_type_tag("foo"))) d;
> >>>
> >>> how does the DWARF for 'a' look like? In particular the iffy thing is
> >>> dwarf_qual_info and the way we check we already have a qualified
> >>> type.
> >>
> >> This is iffy for me as well.
> >>
> >> You are right that basically the desire is to have these tags work like
> >> qualifiers. But of course, attributes are not qualifiers...
> >
> > Note TYPE_ADDR_SPACE has the very same issue (and bugs). There is
> > currently no nice way to stuff in extra "dwarf" qualifiers that are
> > not qualifiers
> > on tree (or the other way around, like we have for TYPE_ADDR_SPACE).
>
> Oh, interesting. I'm not that familiar with TYPE_ADDR_SPACE.
> Will look into it.
>
> >
> >> Currently, the DWARF for 'a' above does not have any annotation DIE at all.
> >> Because the attribute is ignored during parsing (also for 'd'):
> >>
> >> warning: ignoring attributes applied to 'struct S' after definition
> >>
> >> (This happens also if we try to "hide" the struct behind a typedef).
> >>
> >> I am undecided so far whether this is really what we want long term.
> >> Probably not? For now it seems this is not a concern for kernel users;
> >> what they are really concerned about is being able to tag pointer types.
> >> But I do think these type tags are useful beyond the kernel needs, so I'd
> >> like to figure out a better answer long term.
> >>
> >> I have experimented some with the attribute parsing and creating a variant
> >> type of struct S for the attributed version, but not to a working impl.
> >> It's not totally clear to me whether that is something that would be "OK"
> >> to do or would cause breakage elsewhere.
> >>
> >>>
> >>> How do you represent having both the type S and the attributed type S?
> >>> And how do you go between having a const qualified variant of both?
> >>>
> >>
> >> We would need to have two DIEs for the 'struct S'; one with AT_annotation
> >> and one without, as we do for base types like int. Obviously this is not
> >> good if S has many members and/or there are many uses of S with different
> >> tags.
> >>
> >> As for const (or otherwise qualified) versions, it is relatively
> >> straightforward: the DW_TAG_const refers via AT_type either to the
> >> type die for S with AT_annotation or to the one without AT_annotation,
> >> as appropriate.
> >
> > I think what would be good to have is a set of testcases with those
> > corner cases and the expectation what happens.
>
> Yep, makes sense. I will add some tests.
>
> >
> > modified_type_die is unfortunately a twisted maze and I hate making
> > it even more mazier. But I honestly have no energy to try refactoring
> > it into something more maintainable...
>
> I can take a stab at it. I know it will not be easy, but maybe we can
> get some step in the right direction at least. If it's ok with you, I'll
> work on the refactor as a separate/followup patch(es) to this series.
>
> The thing I will need to study more is C++ and other languages'
> interactions with modified_type_die and dwarf2out more generally.
> Some changes I tried for this patch and a while back when originally
> looking at PR110439 (which I still intend to find a solution for,
> maybe as a part of refactoring) seemed to work ok for C but cause
> explosions in C++ tests.
>
> If you have already any ideas or suggestions about the refactor,
> I'm all ears :)
No idea as to how to achieve it but the refactoring goal would be
to separate "typedef-of-X" and "qualified type" handling from a
central entry-point you feed in a tree type to get back a DIE for
it (which currently is what modified_type_die is).
Richard.
> Thanks,
> David
>
> >
> > Richard.
> >
> >>
> >> Thanks for all your comments :)
> >> David
> >>
> >>>> + gen_btf_type_tag_dies (type, attr, mod_die, context_die);
> >>>> + return mod_die;
> >>>> + }
> >>>> else if (code == POINTER_TYPE || code == REFERENCE_TYPE)
> >>>> {
> >>>> dwarf_tag tag = DW_TAG_pointer_type;
> >>>> @@ -22729,6 +22975,7 @@ gen_array_type_die (tree type, dw_die_ref
> >>>> context_die)
> >>>> add_pubtype (type, array_die);
> >>>>
> >>>> add_alignment_attribute (array_die, type);
> >>>> + maybe_gen_btf_type_tag_dies (type, array_die, context_die);
> >>>> }
> >>>>
> >>>> /* This routine generates DIE for array with hidden descriptor, details
> >>>> @@ -23099,6 +23346,8 @@ gen_formal_parameter_die (tree node, tree
> >>>> origin, bool emit_name_p,
> >>>> else
> >>>> {
> >>>> add_child_die (context_die, parm_die);
> >>>> +
> >>>> + gen_btf_decl_tag_dies (node_or_origin, parm_die,
> >>>> context_die);
> >>>> return parm_die;
> >>>> }
> >>>> }
> >>>> @@ -23167,6 +23416,8 @@ gen_formal_parameter_die (tree node, tree
> >>>> origin, bool emit_name_p,
> >>>> gcc_unreachable ();
> >>>> }
> >>>>
> >>>> + gen_btf_decl_tag_dies (node_or_origin, parm_die, context_die);
> >>>> +
> >>>> return parm_die;
> >>>> }
> >>>>
> >>>> @@ -26391,6 +26642,10 @@ gen_tagged_type_die (tree type,
> >>>> else
> >>>> gen_struct_or_union_type_die (type, context_die, usage);
> >>>>
> >>>> + dw_die_ref die = lookup_type_die (type);
> >>>> + if (die)
> >>>> + maybe_gen_btf_type_tag_dies (type, die, context_die);
> >>>> +
> >>>> /* Don't set TREE_ASM_WRITTEN on an incomplete struct; we want to fix
> >>>> it up if it is ever completed. gen_*_type_die will set it for us
> >>>> when appropriate. */
> >>>> @@ -27401,6 +27656,9 @@ gen_decl_die (tree decl, tree origin, struct
> >>>> vlr_context *ctx,
> >>>> break;
> >>>> }
> >>>>
> >>>> + gen_btf_decl_tag_dies (decl_or_origin, lookup_decl_die
> >>>> (decl_or_origin),
> >>>> + context_die);
> >>>> +
> >>>> return NULL;
> >>>> }
> >>>>
> >>>> @@ -33321,6 +33579,9 @@ dwarf2out_early_finish (const char *filename)
> >>>> print_die (comp_unit_die (), dump_file);
> >>>> }
> >>>>
> >>>> + if (btf_tag_htab)
> >>>> + btf_tag_htab->empty ();
> >>>> +
> >>>> /* Generate CTF/BTF debug info. */
> >>>> if ((ctf_debug_info_level > CTFINFO_LEVEL_NONE
> >>>> || btf_debuginfo_p ()) && lang_GNU_C ())
> >>>> @@ -33516,6 +33777,7 @@ dwarf2out_cc_finalize (void)
> >>>> switch_text_ranges = NULL;
> >>>> switch_cold_ranges = NULL;
> >>>> current_unit_personality = NULL;
> >>>> + btf_tag_htab = NULL;
> >>>>
> >>>> early_dwarf = false;
> >>>> early_dwarf_finished = false;
> >>>> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-1.c
> >>>> b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-1.c
> >>>> new file mode 100644
> >>>> index 00000000000..a1c1676a7ba
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-1.c
> >>>> @@ -0,0 +1,11 @@
> >>>> +/* Test simple generation of DW_TAG_GNU_annotation DIE for
> >>>> + btf_decl_tag attribute. */
> >>>> +/* { dg-do compile } */
> >>>> +/* { dg-options "-gdwarf -dA" } */
> >>>> +
> >>>> +int *foo __attribute__((btf_decl_tag ("my_foo")));
> >>>> +
> >>>> +/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\)
> >>>> DW_TAG_GNU_annotation" 1 } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_name: \"btf_decl_tag\"" 1
> >>>> } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_const_value: \"my_foo\"" 1
> >>>> } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_GNU_annotation" 1 } } */
> >>>> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-2.c
> >>>> b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-2.c
> >>>> new file mode 100644
> >>>> index 00000000000..76583840439
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-2.c
> >>>> @@ -0,0 +1,25 @@
> >>>> +/* Test dwarf generation for btf_decl_tag on struct and union members.
> >>>> */
> >>>> +/* { dg-do compile } */
> >>>> +/* { dg-options "-gdwarf -dA" } */
> >>>> +
> >>>> +#define __tag1 __attribute__((btf_decl_tag ("decl1")))
> >>>> +#define __tag2 __attribute__((btf_decl_tag ("decl2")))
> >>>> +
> >>>> +union U {
> >>>> + int i __tag1;
> >>>> + unsigned char ub[4];
> >>>> +};
> >>>> +
> >>>> +struct S {
> >>>> + union U u;
> >>>> + int b __tag2;
> >>>> + char *z __tag1;
> >>>> +};
> >>>> +
> >>>> +struct S my_s __tag1 __tag2;
> >>>> +
> >>>> +/* We must have two occurrances of one of the two annotation DIEs due to
> >>>> + the different attribute sets between declarations above. */
> >>>> +/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\)
> >>>> DW_TAG_GNU_annotation" 3 } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_name: \"btf_decl_tag\"" 3
> >>>> } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_GNU_annotation" 5 } } */
> >>>> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-3.c
> >>>> b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-3.c
> >>>> new file mode 100644
> >>>> index 00000000000..f3fad8fe3d2
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-decl-tag-3.c
> >>>> @@ -0,0 +1,21 @@
> >>>> +/* Test dwarf generation for btf_decl_tag on functions and function
> >>>> args. */
> >>>> +/* { dg-do compile } */
> >>>> +/* { dg-options "-gdwarf -dA" } */
> >>>> +
> >>>> +#define __tag1 __attribute__((btf_decl_tag ("decl1")))
> >>>> +#define __tag2 __attribute__((btf_decl_tag ("decl2")))
> >>>> +
> >>>> +int __tag1 __tag2 func (int arg_a __tag1, int arg_b __tag2)
> >>>> +{
> >>>> + return arg_a * arg_b;
> >>>> +}
> >>>> +
> >>>> +int foo (int x) {
> >>>> + return func (x, x + 1);
> >>>> +}
> >>>> +
> >>>> +/* In this case one of the decl tag DIEs must be duplicated due to
> >>>> differing
> >>>> + DW_AT_GNU_annotation chain between the three uses. */
> >>>> +/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\)
> >>>> DW_TAG_GNU_annotation" 3 } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_name: \"btf_decl_tag\"" 3
> >>>> } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_GNU_annotation" 4 } } */
> >>>> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-1.c
> >>>> b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-1.c
> >>>> new file mode 100644
> >>>> index 00000000000..772aab09cfb
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-1.c
> >>>> @@ -0,0 +1,10 @@
> >>>> +/* Test simple generation for btf_type_tag attribute. */
> >>>> +/* { dg-do compile } */
> >>>> +/* { dg-options "-gdwarf -dA" } */
> >>>> +
> >>>> +int * __attribute__((btf_type_tag("__user"))) ptr;
> >>>> +
> >>>> +/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\)
> >>>> DW_TAG_GNU_annotation" 1 } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_name: \"btf_type_tag\"" 1
> >>>> } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_const_value: \"__user\"" 1
> >>>> } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_GNU_annotation" 1 } } */
> >>>> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-2.c
> >>>> b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-2.c
> >>>> new file mode 100644
> >>>> index 00000000000..9c44e0ee0b9
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-2.c
> >>>> @@ -0,0 +1,31 @@
> >>>> +/* Test that DW_TAG_GNU_annotation DIEs for attribute btf_type_tag are
> >>>> shared
> >>>> + where possible. */
> >>>> +/* { dg-do compile } */
> >>>> +/* { dg-options "-gdwarf -dA" } */
> >>>> +
> >>>> +#define __tag1 __attribute__((btf_type_tag("tag1")))
> >>>> +#define __tag2 __attribute__((btf_type_tag("tag2")))
> >>>> +
> >>>> +int __tag1 foo;
> >>>> +char * __tag1 __tag2 bar;
> >>>> +
> >>>> +struct S
> >>>> +{
> >>>> + unsigned char bytes[8];
> >>>> + unsigned long __tag1 t;
> >>>> + void *ptr;
> >>>> +};
> >>>> +
> >>>> +struct S * __tag1 __tag2 my_S;
> >>>> +
> >>>> +/* Only 2 DW_TAG_GNU_annotation DIEs should be generated, one each for
> >>>> "tag1"
> >>>> + and "tag2", and they should be reused. */
> >>>> +/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\)
> >>>> DW_TAG_GNU_annotation" 2 } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_name: \"btf_type_tag\"" 2
> >>>> } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_const_value: \"tag1\"" 1 }
> >>>> } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_const_value: \"tag2\"" 1 }
> >>>> } */
> >>>> +
> >>>> +/* Each attribute-ed type shall refer via DW_AT_GNU_annotation to the
> >>>> + appropriate annotation DIE, including the annotation DIE for "tag2"
> >>>> which
> >>>> + is always chained to the DIE for "tag1" in this construction. */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_GNU_annotation" 5 } } */
> >>>> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-3.c
> >>>> b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-3.c
> >>>> new file mode 100644
> >>>> index 00000000000..d02144c8004
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-3.c
> >>>> @@ -0,0 +1,15 @@
> >>>> +/* Test dwarf generation for btf_type_tag with cv-quals and typedefs.
> >>>> */
> >>>> +/* { dg-do compile } */
> >>>> +/* { dg-options "-gdwarf -dA" } */
> >>>> +
> >>>> +#define __tag1 __attribute__((btf_type_tag ("tag1")))
> >>>> +#define __tag2 __attribute__((btf_type_tag ("tag2")))
> >>>> +
> >>>> +typedef const int foo;
> >>>> +typedef int __tag1 bar;
> >>>> +
> >>>> +foo __tag2 x;
> >>>> +const bar y;
> >>>> +
> >>>> +/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\)
> >>>> DW_TAG_GNU_annotation" 2 } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_name: \"btf_type_tag\"" 2
> >>>> } } */
> >>>> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-4.c
> >>>> b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-4.c
> >>>> new file mode 100644
> >>>> index 00000000000..2e916e8b443
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-4.c
> >>>> @@ -0,0 +1,33 @@
> >>>> +/* Test generating annotation DIEs for struct/union/enum types. */
> >>>> +/* { dg-do compile } */
> >>>> +/* { dg-options "-gdwarf -dA" } */
> >>>> +
> >>>> +enum E
> >>>> +{
> >>>> + ONE,
> >>>> + TWO
> >>>> +} __attribute__((btf_type_tag("foo")));
> >>>> +
> >>>> +enum E some_e;
> >>>> +
> >>>> +struct S
> >>>> +{
> >>>> + int i;
> >>>> + char c;
> >>>> +} __attribute__((btf_type_tag("foo")));
> >>>> +
> >>>> +typedef struct S S1;
> >>>> +const S1 some_s1;
> >>>> +
> >>>> +union U
> >>>> +{
> >>>> + int x;
> >>>> + char y;
> >>>> +} __attribute__((btf_type_tag("foo")));
> >>>> +
> >>>> +volatile union U volatile_u;
> >>>> +
> >>>> +/* One annotation DIE may be shared by all three annotated types. */
> >>>> +/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\)
> >>>> DW_TAG_GNU_annotation" 1 } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_name: \"btf_type_tag\"" 1
> >>>> } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_GNU_annotation" 3 } } */
> >>>> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-5.c
> >>>> b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-5.c
> >>>> new file mode 100644
> >>>> index 00000000000..1a6b29f99a1
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-btf-type-tag-5.c
> >>>> @@ -0,0 +1,10 @@
> >>>> +/* Test generation for btf_type_tag attribute on array type. */
> >>>> +/* { dg-do compile } */
> >>>> +/* { dg-options "-gdwarf -dA" } */
> >>>> +
> >>>> +int arr[8] __attribute__((btf_type_tag("tagged_arr")));
> >>>> +
> >>>> +/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\)
> >>>> DW_TAG_GNU_annotation" 1 } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_name: \"btf_type_tag\"" 1
> >>>> } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_const_value:
> >>>> \"tagged_arr\"" 1 } } */
> >>>> +/* { dg-final { scan-assembler-times " DW_AT_GNU_annotation" 1 } } */
> >>>> diff --git a/include/dwarf2.def b/include/dwarf2.def
> >>>> index 989f078041d..37b8d6b99d0 100644
> >>>> --- a/include/dwarf2.def
> >>>> +++ b/include/dwarf2.def
> >>>> @@ -174,6 +174,9 @@ DW_TAG (DW_TAG_GNU_formal_parameter_pack, 0x4108)
> >>>> are properly part of DWARF 5. */
> >>>> DW_TAG (DW_TAG_GNU_call_site, 0x4109)
> >>>> DW_TAG (DW_TAG_GNU_call_site_parameter, 0x410a)
> >>>> +
> >>>> +DW_TAG (DW_TAG_GNU_annotation, 0x6001)
> >>>> +
> >>>> /* Extensions for UPC. See: http://dwarfstd.org/doc/DWARF4.pdf. */
> >>>> DW_TAG (DW_TAG_upc_shared_type, 0x8765)
> >>>> DW_TAG (DW_TAG_upc_strict_type, 0x8766)
> >>>> @@ -456,6 +459,7 @@ DW_AT (DW_AT_GNU_pubtypes, 0x2135)
> >>>> DW_AT (DW_AT_GNU_discriminator, 0x2136)
> >>>> DW_AT (DW_AT_GNU_locviews, 0x2137)
> >>>> DW_AT (DW_AT_GNU_entry_view, 0x2138)
> >>>> +DW_AT (DW_AT_GNU_annotation, 0x2139)
> >>>> /* VMS extensions. */
> >>>> DW_AT (DW_AT_VMS_rtnbeg_pd_address, 0x2201)
> >>>> /* GNAT extensions. */
> >>>> --
> >>>> 2.47.2
> >>>>
> >>
>
