On 6/30/25 06:11, Richard Biener wrote:
>> +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. Global variable
>> + declarations may reach this function more than once. */
>> + DECL_ATTRIBUTES (t)
>> + = remove_attribute ("btf_decl_tag", DECL_ATTRIBUTES (t));
> I do not like modifying trees as part of dwarf2out. You should be able to
> see whether a DIE already has the respective attribute applied?
Yes, you're right. For decl_tag the case is simple and better handled by
consulting the hash table. Simple fix and this remove_attribute can be
deleted.
Understood re: modifying trees in dwarf2out. I agree it's not ideal.
For this case the remove_attribute can be deleted. For the two below,
one is already immediately restored and the other could be as well so
that there are no lasting changes in the tree at all.
I will explain the reasoning some more below.
>
>> +}
>> +
>> /* 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
>> @@ -13674,6 +13894,7 @@ modified_type_die (tree type, int cv_quals, bool
>> reverse,
>> tree item_type = NULL;
>> tree qualified_type;
>> tree name, low, high;
>> + tree tags;
>> dw_die_ref mod_scope;
>> struct array_descr_info info;
>> /* Only these cv-qualifiers are currently handled. */
>> @@ -13783,10 +14004,62 @@ 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);
>> + {
>> + tree dtags = lookup_attribute ("btf_type_tag",
>> + TYPE_ATTRIBUTES (dtype));
>> + if ((tags = lookup_attribute ("btf_type_tag",
>> + TYPE_ATTRIBUTES (type)))
>> + && !attribute_list_equal (tags, dtags))
>> + {
>> + /* Use of a typedef with additional btf_type_tags.
>> + Create a new typedef DIE to which we can attach the
>> + additional type_tag DIEs without disturbing other users
>> of
>> + the underlying typedef. */
>> + dw_die_ref mod_die = modified_type_die (dtype, cv_quals,
>> + reverse,
>> context_die);
>> + 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);
>> +
>> + /* 'tags' is an accumulated list of type_tag attributes
>> + for the typedef'd type on both sides of the typedef.
>> + 'dtags' is the set of type_tag attributes only appearing
>> + in the typedef itself.
>> + Find the set of type_tags only on the _use_ of the
>> + typedef, i.e. (tags - dtags). By construction these
>> + additional type_tags have been chained onto the head of
>> + the attribute list of the original typedef. */
>> + tree t = tags;
>> + bool altered_chain = false;
>> + while (t)
>> + {
>> + if (TREE_CHAIN (t) == dtags)
>> + {
>> + TREE_CHAIN (t) = NULL_TREE;
>> + altered_chain = true;
>> + break;
>> + }
>> + t = TREE_CHAIN (t);
>> + }
> And this might be part of that and should thus split out, or be part
> of gen_btf_type_tag_dies itself? I expected the hashtable to
> cover the DIEs we have for existing chains of tags?
OK, this can move into gen_btf_type_tag_dies.
The reason the hash table doesn't help us here is that the uncertainty
is in the attribute list itself.
Specifically, if the type of a variable is a typedef with an additional
type attribute applied, then it is unclear from just the tree node of
the type of the variable whether that attribute is "new" on the use of
the typedef, or is part of the typedef itself.
For example:
typedef int __attribute__((btf_type_tag ("tag1"))) foo;
foo __attribute__((btf_type_tag ("tag2"))) x;
For the typedef, we create a DW_TAG_typedef DIE with name "foo", with
AT_type referring to a base type DIE for an integer type with "tag1"
attached via AT_GNU_annotation. So far so good.
When generating dwarf for variable 'x', the tree node we have for
the type of 'x' is an integer type node with name "foo" but the attribute
list contains _both_ "tag2" and "tag1". This is what I mean with the
comment in the code above about the "accumulated list" from both sides
of the typedef. It happens because we have a variant type node of
the named type foo created, in order to be a distinct node for
(foo + "tag2") for the type of 'x'.
IOW, 'dtags' holds "tag1" since it comes from the typedef itself,
and 'tags' holds both "tag1" and "tag2". We need to do some extra work
looking at the tree to figure out which type_tags are actually on the
_use_ of the typedef vs. part of the typdef itself, in order to correctly
decide which chains of tags should be associated to which DIE.
Note that the attribute chain in the tree is immediately restored
after generating the tag dies...
>
>> +
>> + /* Now generate the type_tag DIEs only for the new
>> + type_tags appearing in the use of the typedef, and
>> + attach them to the cloned typedef DIE. */
>> + gen_btf_type_tag_dies (type, tags, mod_die, context_die);
>> +
>> + /* Restore the list of type_tags. In principle, the same
>> + attribute list could be shared by unrelated types. */
>> + if (altered_chain)
>> + TREE_CHAIN (t) = dtags;
...here. So there are no lasting changes in the tree.
It could be done w/o altering the tree at all by duplicating the relevant
attribute list and operating on that, but it seems a little wasteful.
>> +
>> + 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 +14160,17 @@ modified_type_die (tree type, int cv_quals, bool
>> reverse,
>> first_quals |= dwarf_qual_info[i].q;
>> }
>> }
>> + else if ((tags = lookup_attribute ("btf_type_tag", TYPE_ATTRIBUTES
>> (type))))
>> + {
>> + /* 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));
> See above.
The reason for this one is simply the issue of recursion.
Doing remove_attribute here is analogous to the cv-qual handling using
get_nearest_type_subqualifiers and recursing with the lesser qualified
type. In this case the lesser qualified type is the same type without
the type_tag attribute(s)
I think the alternative would be to split the type_tags into a separate
argument to modified_type_die, similar to cv_quals, which seemed like
a much more invasive set of changes to make.
>
>> + dw_die_ref mod_die = modified_type_die (type, cv_quals, reverse,
>> + context_die);
>> + gen_btf_type_tag_dies (type, tags, mod_die, context_die);
It should work fine to save TYPE_ATTRIBUTES above and restore it to the
original here after the recursive call so that ultimately the tree
is unchanged.
Would the same approach with that modification be OK?
>> + return mod_die;
>> + }
>> else if (code == POINTER_TYPE || code == REFERENCE_TYPE)
>> {
>> dwarf_tag tag = DW_TAG_pointer_type;
>> @@ -22770,6 +23054,7 @@ gen_array_type_die (tree type, dw_die_ref
>> context_die)
