From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) by sourceware.org (Postfix) with ESMTPS id DDEA33857C4C for ; Thu, 22 Jul 2021 11:44:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DDEA33857C4C Received: by mail-ej1-x635.google.com with SMTP id hr1so7971923ejc.1 for ; Thu, 22 Jul 2021 04:44:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Ch9ZwPUAmCWFZE8BvQpwqwVDte/RpqmtMxvcdhtDKVQ=; b=nTIQY414a0YNohsJFGZsH1+NQ1ZvnVhWMziLwaGj0olLNS5Isp0yqPhxJsjk6gjNx6 04K1rmIVO9UivaK5+ojSDMiipUloTQprjQiIojyciJ9GKcOwaPOR+QemA+BIHI1GDKIB nnL2XQHk3H8PgQbhsTkrTNPRlBHcLn/Fx/j1Dvumf7lWWlY1WkGURO2U9RbzC/fDSULJ 8r75ifTbslvV5pmzRuyQCEtKb4CmJyLopD4OjGZqb7/+OH+ZE2U4r8rp4Rk8H+BNLJrX k0uF+unFzvohN19JS0zUpYstotFbOG24M+jA5JLbTHER1O1HZtydft0x9lOHr28U7otO DUOg== X-Gm-Message-State: AOAM531bOka/OUFyimh1/nfYnp5CUqMzf89LAu/OXQqwikA1CoZo0aCG Wv/4bO/+K5cobw60H/gHpBMEY1lkem4czvLPAso= X-Google-Smtp-Source: ABdhPJy4pCWurOOW+l1whvF6dqfilxZ9d6dfkN4N9HiKHhyWpXMUNZttZvYaTPO24KRYipqSMO9Ft8TSRI3oZzUb8rE= X-Received: by 2002:a17:906:c29a:: with SMTP id r26mr44149989ejz.235.1626954239954; Thu, 22 Jul 2021 04:43:59 -0700 (PDT) MIME-Version: 1.0 References: <20210701151657.935006-1-abidh@codesourcery.com> <8735sfamu7.fsf@dem-tschwing-1.ger.mentorg.com> <50d1657e-c5de-9cb9-1aaa-233f27dfe9b9@mentor.com> <210da16b-5992-7d7f-7223-4a4f08998d49@mentor.com> <6C66F8F4-71EC-431E-A0C5-737BBB9D84F3@gmail.com> <28f299d1-93d5-ff33-1a83-41f373d3d1f7@mentor.com> In-Reply-To: <28f299d1-93d5-ff33-1a83-41f373d3d1f7@mentor.com> From: Richard Biener Date: Thu, 22 Jul 2021 13:43:49 +0200 Message-ID: Subject: Re: [PATCH] [DWARF] Fix hierarchy of debug information for offload kernels. To: Hafiz Abid Qadeer Cc: Thomas Schwinge , Abid Qadeer , Jakub Jelinek , GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Jul 2021 11:44:02 -0000 On Wed, Jul 21, 2021 at 7:55 PM Hafiz Abid Qadeer wrote: > > On 19/07/2021 17:41, Richard Biener wrote: > > On July 19, 2021 6:13:40 PM GMT+02:00, Hafiz Abid Qadeer wrote: > >> On 19/07/2021 11:45, Richard Biener wrote: > >>> On Fri, Jul 16, 2021 at 10:23 PM Hafiz Abid Qadeer > >>> wrote: > >>>> > >>>> On 15/07/2021 13:09, Richard Biener wrote: > >>>>> On Thu, Jul 15, 2021 at 12:35 PM Hafiz Abid Qadeer > >>>>> 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 ("") > >>>>> > >>>>> 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. I think you should consult with gdb folks on this - the functions are still children of the enclosing function as seen in the abstract instance. Just the concrete instance is put in another place. But yes, that was what I expected as bad side-effect of the change. Now I wonder how to fix that - even for offloading a "good" debugger could allow debugging both the host and the target and DTRT when printing a variable from the containing function on the target (lookup the variable on the host). So I think we need to get to an agreement between the debug info producer and consumer here. Usually the DWARF spec is not of much help here. Richard. > 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