Re: [PAHOLE v3 2/3] dwarf_loader: Add support for DW_TAG_GNU_annotation

[Vineet Gupta <vineet.gupta@linux.dev>]

Hi Emil,

On 6/1/26 6:04 PM, Emil Tsalapatis wrote:
>>   }
>>   
>> +static bool die__tag_is_annotation(Dwarf_Die *die)
>> +{
>> +	unsigned int tag = dwarf_tag(die);
>> +	return tag == DW_TAG_LLVM_annotation || tag == DW_TAG_GNU_annotation;
> Can we also have a shorthand static inline for
>
> tag == DW_TAG_LLVM_annotation || tag == DW_TAG_GNU_annotation
>
> ? Would be nice in a bunch of places.

OK.
>
>> +}
>> +
>>   static int add_llvm_annotation(Dwarf_Die *die, int component_idx, struct conf_load *conf,
>>   			       struct list_head *head)

This one is used by both so I renamed it to add_tag_annotation.

>>   {
>> @@ -943,7 +949,7 @@ static int add_child_llvm_annotations(Dwarf_Die *die, int component_idx,
> Since this is not just LLVM annotations can we rename it to
> *_tag_annotations or something similar? Same for all other llvm-named
> fucntions we are explicitly handling GCC tags in.

This is llvm specific, since the child style annotations are generated 
by llvm only. So the current name makes sense.

>>   	die = &child;
>>   	do {
>> -		if (dwarf_tag(die) == DW_TAG_LLVM_annotation) {
>> +		if (die__tag_is_annotation(die)) {
>>   			ret = add_llvm_annotation(die, component_idx, conf, head);
>>   			if (ret)
>>   				return ret;
>> @@ -953,6 +959,35 @@ static int add_child_llvm_annotations(Dwarf_Die *die, int component_idx,
>>   	return 0;
>>   }
>>   
>> +/* Handle gcc sytle btf_decl_tag annotations for functions/struct/member tags
> Typo: sytle -> style

Fixed.

>> + * Pointers are handled seperately, inline in die__create_new_pointer_tag ()
> Typo: seperately -> separately, also maybe remove the space before the ()

Fixed.

>> + */
>> +static int add_gnu_annotation_chain(Dwarf_Die *die, int component_idx,
>> +				    struct conf_load *conf, struct list_head *head)
>> +{
>> +	Dwarf_Attribute attr;
>> +	Dwarf_Die annot_die;
>> +
>> +	if (dwarf_attr(die, DW_AT_GNU_annotation, &attr) == NULL ||
>> +	    dwarf_formref_die(&attr, &annot_die) == NULL)
>> +		return 0;
>> +
>> +	for (;;) {
>> +		if (dwarf_tag(&annot_die) != DW_TAG_GNU_annotation)
>> +			break;
>> +
>> +		int ret = add_llvm_annotation(&annot_die, component_idx, conf, head);
> E.g., here imo it makes sense to keep the llvm name since AFAICT this is
> one of the places where we normalize the GCC tags.

Normalization is just an internal detail, this is actually using the 
common functionality which we already renamed above to the agnostic name 
add_tag_annotation.

>> -	/* 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)
>> +	/* If skipping btf_type_tag encoding, just create a new tag, return */
>> +	if (conf->skip_encoding_btf_type_tag)
>>   		return tag__new(die, cu);
>>   
>> -	/* Otherwise, check DW_TAG_LLVM_annotation child tags */
>> +	/* Handle LLVM style annotation tags if present */
> Can we move this comment to right below the goto? It's slightly
> confusing here.

Yep makes sense. Also for consistency did the same for the GNU comment.

> For that matter, why are we processing LLVM and GNU tags in succession?
> Could we just turn check_gnu_attr into a separate function and instead
> of a goto do
>
> return check_gnu_attr();
>
> ?
>
> Then again mixing the two types of tags seems theoretically possible
> and the least surprising thing to do is handle both.

Code to handle both needs to exist in some shape and form, we just do it 
one after other, rather then in an if .. else.
If LLVM style handling happens, @tag at the end of llvm loop will be non 
null and it won't enter the gnu block, its clear enough from looking at 
the code IMO.
Carving out another function feels a bit excessive.

>>   		}
>> diff --git a/dwarves.h b/dwarves.h
>> index 5ec16e750e83..42b8e39aa2dd 100644
>> --- a/dwarves.h
>> +++ b/dwarves.h
>> @@ -670,7 +670,8 @@ static inline int tag__is_tag_type(const struct tag *tag)
>>   	       tag->tag == DW_TAG_volatile_type ||
>>   	       tag->tag == DW_TAG_atomic_type ||
>>   	       tag->tag == DW_TAG_unspecified_type ||
>> -	       tag->tag == DW_TAG_LLVM_annotation;
>> +	       tag->tag == DW_TAG_LLVM_annotation ||
>> +	       tag->tag == DW_TAG_GNU_annotation;
> Another place where we can just have an "is annotation tag" predicate.

Fixed.

>
>> @@ -731,7 +734,7 @@ static type_id_t skip_llvm_annotations(const struct cu *cu, type_id_t id)
> Rename this function to not be llvm-specific anymore?

OK.

>
>>   		if (id == 0)
>>   			break;
>>   		type = cu__type(cu, id);
>> -		if (type == NULL || type->tag != DW_TAG_LLVM_annotation || type->type == id)
>> +		if (type == NULL || (type->tag != DW_TAG_LLVM_annotation && type->tag != DW_TAG_GNU_annotation) || type->type == id)
> Same as before wrt predicate

OK.

Thx for the good feedback.
-Vineet