Re: [PAHOLE Patch v2 2/2] Add support for DW_TAG_GNU_annotation

On 5/29/26 16:32, Vineet Gupta wrote:
> On 5/29/26 06:37, Alan Maguire wrote:
>> thanks for this Vineet! I'm building gcc-16 to manually test now but 
>> I have a few
>> suggestions below in the meantime. Changes look good in CI [1]
>>
>> [1] https://github.com/alan-maguire/dwarves/actions/runs/26632028538
>
> Cool.
>>> +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);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        if (dwarf_attr(&annot_die, DW_AT_GNU_annotation, &attr) == 
>>> NULL ||
>>> +            dwarf_formref_die(&attr, &annot_die) == NULL)
>>> +            break;
>>> +    }
>> the pointer tag handling below uses bookkeeping to track visited 
>> dies; is there a need
>> for that here too? maybe a bit paranoid but perhaps we could haul the 
>> loop detection
>> logic out and reuse in both places?
>
> Looking back now, the loop detection seems excessive. It was from the 
> initial attempt to triage what seemed like an infinite loop in pahole 
> but just turned out to be excessive match failures and bails and 
> retries due to the gcc bug [1] which was generating variants of same 
> core struct, causing failures pretty much everywhere due to the core 
> struct being embedded all over. If we want to add loop detection that 
> should be addon as it unconditional additional processing. I agree 
> that even with that out of picture this function and the opencoded 
> check_gnu_attr code seem to share a lot of structure - but they have 
> their own *thing* to do and it would be unwieldy and awkward to factor 
> out into a shared loop body with a call back or an additional arg to 
> call add_llvm_annotation or die__add_btf_type_tag for either of those 
> cases.
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=125421 
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=125421>
>
>>> +check_gnu_attr:
>>> +    /* Check for GCC-style DW_AT_GNU_annotation attribute */
>>> +    if (tag != NULL ||
>>> +        dwarf_attr(die, DW_AT_GNU_annotation, &attr) == NULL ||
>>> +        dwarf_formref_die(&attr, &annot_die) == NULL)
>>> +        goto out;
>>> +
>>> +    Dwarf_Off visited[256];
>>> +    int nr_visited = 0;
>>> +
>>> +    for (;;) {
>>> +        Dwarf_Off off = dwarf_dieoffset(&annot_die);
>>> +        bool cycle = false;
>>> +        int i;
>>> +
>>> +        for (i = 0; i < nr_visited; i++) {
>>> +            if (visited[i] == off) {
>>> +                cycle = true;
>>> +                break;
>>> +            }
>>> +        }
>>> +        if (cycle || nr_visited >= (int)ARRAY_SIZE(visited))
>>> +            break;
>>> +        visited[nr_visited++] = off;
>
> I'm thinking of ripping this all out. Rest of dwarf machinery feels 
> robust enough.
>
>>> +
>>> +        if (dwarf_tag(&annot_die) != DW_TAG_GNU_annotation)
>>> +            break;
>>> +
>>> +        name = attr_string(&annot_die, DW_AT_name, conf);
>>> +        if (strcmp(name, "btf_type_tag") != 0)
>>> +            break;
>> should we "continue;" here instead; if we encounter a non-type tag 
>> annotation
>> might we miss subsequent btf type tag annotations? might not be an 
>> issue today
>> but might be more future-proof to continue here maybe?
>
> Good point. Will do.

Claude suggests break is the right thing to do for precisely the reason 
you mention.
This code only handles DW_TAG_pointer_type DIEs which currently don't 
expect non-type tag, and this pertains to a single chain it could point 
to a potential issue as of now.
But up to you what you feel is better.

Thx,
-Vineet