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

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

On 6/2/26 11:14 AM, Arnaldo Carvalho de Melo wrote:
> 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 <vineet.gupta@linux.dev>
>>> ---
>>> 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 <emil@etsalapatis.com>
>   
>> 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.

Thx, I already spotted that and fixed for v4.

>
>>> +	}
>>> +
>>> +	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 :)

Ok will fix it.

Thx,
-Vineet