Re: [PATCH] [DWARF] Fix hierarchy of debug information for offload kernels.

[Hafiz Abid Qadeer <abid_qadeer@mentor.com>]

On 19/07/2021 17:41, Richard Biener wrote:
> On July 19, 2021 6:13:40 PM GMT+02:00, Hafiz Abid Qadeer <abid_qadeer@mentor.com> wrote:
>> On 19/07/2021 11:45, Richard Biener wrote:
>>> On Fri, Jul 16, 2021 at 10:23 PM Hafiz Abid Qadeer
>>> <abid_qadeer@mentor.com> wrote:
>>>>
>>>> On 15/07/2021 13:09, Richard Biener wrote:
>>>>> On Thu, Jul 15, 2021 at 12:35 PM Hafiz Abid Qadeer
>>>>> <abid_qadeer@mentor.com> wrote:
>>>>>>
>>>>>> On 15/07/2021 11:33, Thomas Schwinge wrote:
>>>>>>>
>>>>>>>> Note that the "parent" should be abstract but I don't think
>> dwarf has a
>>>>>>>> way to express a fully abstract parent of a concrete instance
>> child - or
>>>>>>>> at least how GCC expresses this causes consumers to
>> "misinterpret"
>>>>>>>> that.  I wonder if adding a DW_AT_declaration to the late DWARF
>>>>>>>> emitted "parent" would fix things as well here?
>>>>>>>
>>>>>>> (I suppose not, Abid?)
>>>>>>>
>>>>>>
>>>>>> Yes, adding DW_AT_declaration does not fix the problem.
>>>>>
>>>>> Does emitting
>>>>>
>>>>> DW_TAG_compile_unit
>>>>>   DW_AT_name    ("<artificial>")
>>>>>
>>>>>   DW_TAG_subprogram // notional parent function (foo) with no code
>> range
>>>>>     DW_AT_declaration 1
>>>>> a:    DW_TAG_subprogram // offload function foo._omp_fn.0
>>>>>       DW_AT_declaration 1
>>>>>
>>>>>   DW_TAG_subprogram // offload function
>>>>>   DW_AT_abstract_origin a
>>>>> ...
>>>>>
>>>>> do the trick?  The following would do this, flattening function
>> definitions
>>>>> for the concrete copies:
>>>>>
>>>>> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
>>>>> index 82783c4968b..a9c8bc43e88 100644
>>>>> --- a/gcc/dwarf2out.c
>>>>> +++ b/gcc/dwarf2out.c
>>>>> @@ -6076,6 +6076,11 @@ maybe_create_die_with_external_ref (tree
>> decl)
>>>>>    /* Peel types in the context stack.  */
>>>>>    while (ctx && TYPE_P (ctx))
>>>>>      ctx = TYPE_CONTEXT (ctx);
>>>>> +  /* For functions peel the context up to namespace/TU scope.  The
>> abstract
>>>>> +     copies reveal the true nesting.  */
>>>>> +  if (TREE_CODE (decl) == FUNCTION_DECL)
>>>>> +    while (ctx && TREE_CODE (ctx) == FUNCTION_DECL)
>>>>> +      ctx = DECL_CONTEXT (ctx);
>>>>>    /* Likewise namespaces in case we do not want to emit DIEs for
>> them.  */
>>>>>    if (debug_info_level <= DINFO_LEVEL_TERSE)
>>>>>      while (ctx && TREE_CODE (ctx) == NAMESPACE_DECL)
>>>>> @@ -6099,8 +6104,7 @@ maybe_create_die_with_external_ref (tree
>> decl)
>>>>>         /* Leave function local entities parent determination to
>> when
>>>>>            we process scope vars.  */
>>>>>         ;
>>>>> -      else
>>>>> -       parent = lookup_decl_die (ctx);
>>>>> +      parent = lookup_decl_die (ctx);
>>>>>      }
>>>>>    else
>>>>>      /* In some cases the FEs fail to set DECL_CONTEXT properly.
>>>>>
>>>>
>>>> Thanks. This solves the problem. Only the first hunk was required.
>> Second hunk
>>>> actually causes an ICE when TREE_CODE (ctx) == BLOCK.
>>>> OK to commit the attached patch?
>>>
>>> I think we need to merge the TYPE_P peeling and FUNCTION_DECL peeling
>> into
>>> one loop since I suppose we can have a nested function in class
>> scope.
>>> So sth like
>>>
>>> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
>>> index 82783c4968b..61228410b51 100644
>>> --- a/gcc/dwarf2out.c
>>> +++ b/gcc/dwarf2out.c
>>> @@ -6073,8 +6073,12 @@ maybe_create_die_with_external_ref (tree decl)
>>>      }
>>>    else
>>>      ctx = DECL_CONTEXT (decl);
>>> -  /* Peel types in the context stack.  */
>>> -  while (ctx && TYPE_P (ctx))
>>> +  /* Peel types in the context stack.  For functions peel the
>> context up
>>> +     to namespace/TU scope.  The abstract copies reveal the true
>> nesting.  */
>>> +  while (ctx
>>> +        && (TYPE_P (ctx)
>>> +            || (TREE_CODE (decl) == FUNCTION_DECL
>>> +                && TREE_CODE (ctx) == FUNCTION_DECL)))
>>>      ctx = TYPE_CONTEXT (ctx);
>>>    /* Likewise namespaces in case we do not want to emit DIEs for
>> them.  */
>>>    if (debug_info_level <= DINFO_LEVEL_TERSE)
>>>
>> This causes an ICE,
>> internal compiler error: tree check: expected class 'type', have
>> 'declaration' (function_decl)
>>
>> Did you intend something like this:
>>
>> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
>> index 561f8b23517..c61f0041fba 100644
>> --- a/gcc/dwarf2out.c
>> +++ b/gcc/dwarf2out.c
>> @@ -6121,3 +6121,8 @@ maybe_create_die_with_external_ref (tree decl)
>> -  /* Peel types in the context stack.  */
>> -  while (ctx && TYPE_P (ctx))
>> -    ctx = TYPE_CONTEXT (ctx);
>> +  /* Peel types in the context stack.  For functions peel the context
>> up
>> +     to namespace/TU scope.  The abstract copies reveal the true
>> nesting.  */
>> +  while (ctx
>> +       && (TYPE_P (ctx)
>> +           || (TREE_CODE (decl) == FUNCTION_DECL
>> +               && TREE_CODE (ctx) == FUNCTION_DECL)))
>> +    ctx = TYPE_P (ctx) ? TYPE_CONTEXT (ctx) : DECL_CONTEXT (ctx);
>> +
> 
> Yes, of course. 
> 
>>
>>> if that works it's OK.  Can you run it on the gdb testsuite with
>> -flto added
>>> as well please (you need to do before/after comparison since IIRC
>> adding
>>> -flto will add a few fails).

GDB testsuite shows some extra fails which mainly happen because testcase assumes that you can
access the local variable of enclosing function from the nested function (or omp parallel region).
After this change, the nested functions are no longer children of the enclosing function so those
tests fail.

The problem that prompted this patch happened for parent function that did not have a code range i.e
a notional parent.  I was wondering if we should update the ctx only for such parents instead of all
function as we did above.

Thanks,
-- 
Hafiz Abid Qadeer
Mentor, a Siemens Business