Re: [PAHOLE v3 1/3] dwarf_loader: Extract die__add_btf_type_tag() helper

On Mon, Jun 01, 2026 at 03:27:41PM -0400, Emil Tsalapatis wrote:
> On Mon Jun 1, 2026 at 2:35 PM EDT, Vineet Gupta wrote:
> > NFC change preparing for DW_TAG_GNU_annotation support.
> > Extract the btf_type_tag annotation creation logic from into a helper
> > die__add_btf_type_tag().
> >
> > Signed-off-by: Vineet Gupta <[email protected]>
> > ---
> > Changes since v2 [2]
> >  - die__add_btf_type_tag() returns pointer not error code.
> >
> > Changes since v1 [1]
> >  - NFC reinstate some original comments
> 
> Reviewed-by: Emil Tsalapatis <[email protected]>
 
> Minor nit/question below.

Some more nits below :-)
 
> > +++ b/dwarf_loader.c
> > @@ -1600,14 +1600,43 @@ static struct btf_type_tag_type *die__create_new_btf_type_tag_type(Dwarf_Die *di
> >  	return tag;
> >  }
> >  
> > +static struct btf_type_tag_ptr_type *die__add_btf_type_tag(struct btf_type_tag_ptr_type *tag,
> > +							    Dwarf_Die *die, Dwarf_Die *adie,
> > +							    struct cu *cu, struct conf_load *conf)
> > +{
> > +	struct btf_type_tag_type *annot;
> > +	uint32_t id;
> > +
> > +	if (tag == NULL) {
> > +		tag = die__create_new_btf_type_tag_ptr_type(die, cu);
> > +		if (!tag)
> > +			return NULL;

There was an inconsistency on the code you moved here, namely first tag
is tested against NULL then it is negated, equivalent, but since we're
moving it, lets make it consistent, in the kernel it is most common to
use !ptr as it is more compact, so please make the first test !tag, just
like the second.

> > +	}
> > +
> > +	annot = die__create_new_btf_type_tag_type(adie, cu, conf);
> > +	if (annot == NULL)
> > +		return NULL;
> > +
> > +	if (cu__table_add_tag(cu, &annot->tag, &id) < 0)
> > +		return NULL;
> > +
> > +	struct dwarf_tag *dtag = tag__dwarf(&annot->tag);
> > +	dtag->small_id = id;
> > +	cu__hash(cu, &annot->tag);
> > +
> > +	/* Prepends: for annotations tag1 -> tag2 -> tag3,
 
> Not familiar with pahole's coding style, do we want to adjust the
> comments?

Not that important, but we try to follow kernel style, so yeah that
would be:

	/*
	 * Prepends: for annotations tag1 -> tag2 -> tag3,

But by now I don't dwell that much on these minor details :)

> > +	 * the tag->tags list ends up as tag3 -> tag2 -> tag1.
> > +	 */
> > +	list_add(&annot->node, &tag->tags);
> > +	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
> > @@ -1627,29 +1656,9 @@ static struct tag *die__create_new_pointer_tag(Dwarf_Die *die, struct cu *cu,
> >  		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)
> > +		tag = die__add_btf_type_tag(tag, die, cdie, cu, conf);
> > +		if (tag == NULL)
> >  			return NULL;
> > -
> > -		struct dwarf_tag *dtag = tag__dwarf(&annot->tag);
> > -		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);