From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id DEFC53896C30 for ; Mon, 12 Apr 2021 14:50:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DEFC53896C30 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 13CEoiUv003093 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Apr 2021 10:50:49 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 13CEoiUv003093 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 3B1301E789; Mon, 12 Apr 2021 10:50:44 -0400 (EDT) Subject: Re: [PATCH] gdb/dwarf2: fix "info locals" for clang-compiled inlined functions To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: <1613404301-26458-1-git-send-email-tankut.baris.aktemur@intel.com> From: Simon Marchi Message-ID: <68590dc2-3bbf-6c96-4d4a-f3dc55323aa6@polymtl.ca> Date: Mon, 12 Apr 2021 10:50:43 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <1613404301-26458-1-git-send-email-tankut.baris.aktemur@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 12 Apr 2021 14:50:44 +0000 X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Apr 2021 14:50:54 -0000 On 2021-02-15 10:51 a.m., Tankut Baris Aktemur via Gdb-patches wrote: > GDB reports duplicate local vars with "" values for > inlined functions that are compiled with Clang. > > Suppose we have > > __attribute__((always_inline)) > static void aFunction() { > int a = 42; > if(a > 2) { > int value = a; > value += 10; /* break here */ > } > } > > The "info locals" command at the "break here" line gives the following > output: > > ... > Breakpoint 1, aFunction () at test.c:6 > 6 value += 10; /* break here */ > (gdb) info locals > value = 42 > a = 42 > value = > (gdb) > > The reason is, inlined functions that are compiled by Clang do not > contain DW_AT_abstract_origin attributes in the DW_TAG_lexical_block > entries. E.g. the DIE of the inlined function above is > > 0x00000087: DW_TAG_inlined_subroutine > DW_AT_abstract_origin (0x0000002a "aFunction") > DW_AT_low_pc (0x00000000004004b2) > DW_AT_high_pc (0x00000000004004d2) > DW_AT_call_file ("/tmp/test.c") > DW_AT_call_line (11) > DW_AT_call_column (0x03) > > 0x0000009b: DW_TAG_variable > DW_AT_location (DW_OP_fbreg -4) > DW_AT_abstract_origin (0x00000032 "a") > > 0x000000a3: DW_TAG_lexical_block > DW_AT_low_pc (0x00000000004004c3) > DW_AT_high_pc (0x00000000004004d2) > > 0x000000b0: DW_TAG_variable > DW_AT_location (DW_OP_fbreg -8) > DW_AT_abstract_origin (0x0000003e "value") > > This causes GDB to fail matching the concrete lexical scope with the > corresponding abstract entry. Hence, the locals vars of the abstract > function that are contained in the lexical scope are read separately > (and thus, in addition to) the locals vars of the concrete scope. > Because the abstract definition of the vars do not contain location > information, we see the extra 'value = ' above. > > This bug is highly related to PR gdb/25695, but the root cause is not > exactly the same. In PR gdb/25695, GCC emits an extra > DW_TAG_lexical_block without an DW_AT_abstract_origin that wraps the > body of the inlined function. That is, the trees of the abstract DIE > for the function and its concrete instance are structurally not the > same. In the case of using Clang, the trees have the same structure. > > To tackle the Clang case, when traversing the children of the concrete > instance root, keep a reference to the child of the abstract DIE that > corresponds to the concrete child, so that we can match the two DIEs > heuristically in case of missing DW_AT_abstract_origin attributes. > > The updated gdb.opt/inline-locals.exp test has been checked with GCC > 5-10 and Clang 5-11. Hi Baris, In addition to updating this test, could you try to make a test using the DWARF assembler in gdb.dwarf2/ ? The next clang version might not generate this "bug" anymore, but we still want to make sure this behavior keeps working. Also, it would make sure the behavior is tested even when running the testsuite with gcc (which most people do, because it's the default). Speaking of "bug", do you think it's something that should be reported to clang so they can fix their output? If there's already a clang bug open for this, then can you add it to the commit message? Otherwise, the patch looks good to me, see minor comments below. > > gdb/ChangeLog: > 2021-02-15 Tankut Baris Aktemur > > * dwarf2/read.c (inherit_abstract_dies): Keep a reference to the > corresponding child of the abstract DIE when iterating the > children of the concrete DIE. > > gdb/testsuite/ChangeLog: > 2021-02-15 Tankut Baris Aktemur > > * gdb.opt/inline-locals.c (scoped): New function. > (main): Call 'scoped'. > * gdb.opt/inline-locals.exp: Update with "info locals" tests > for scoped variables. > --- > gdb/dwarf2/read.c | 48 ++++++++++++++++++++++++- > gdb/testsuite/gdb.opt/inline-locals.c | 20 +++++++++++ > gdb/testsuite/gdb.opt/inline-locals.exp | 30 ++++++++++++++++ > 3 files changed, 97 insertions(+), 1 deletion(-) > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index 51bf0fbeea5..11696cebd96 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -13536,6 +13536,36 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu) > sect_offset_str (die->sect_off), > sect_offset_str (origin_die->sect_off)); > > + /* Find if the concrete and abstract trees are structurally the > + same. This is a shallow traversal and it is not bullet-proof; > + the compiler can trick the debugger into believing that the trees > + are isomorphic, whereas they actually are not. However, the > + likelyhood of this happening is pretty low, I think, and a > + full-fledged check would be an overkill. */ Just a nit: remove the "I think". I think [:)] that first-person in comments sounds weird, because once this is merged, this is no longer you speaking, it's the code. But I agree with you, I think it's is an acceptable assumption. Especially since you restrict the fix to DW_TAG_lexical_block DIEs. > + bool are_isomorphic = true; > + die_info *concrete_child = die->child; > + die_info *abstract_child = origin_die->child; > + while (concrete_child != nullptr || abstract_child != nullptr) > + { > + if (concrete_child == nullptr > + || abstract_child == nullptr > + || concrete_child->tag != abstract_child->tag) > + { > + are_isomorphic = false; > + break; > + } > + > + concrete_child = concrete_child->sibling; > + abstract_child = abstract_child->sibling; > + } > + > + /* Walk the origin's children in parallel to the concrete children. > + This helps match an origin child in case the debug info misses > + DW_AT_abstract_origin attributes. Keep in mind that the abstract > + origin tree may not have the same tree structure as the concrete > + DIE, though. */ > + die_info *corresponding_abstract_child = origin_die->child; I'd suggest leaving corresponding_abstract_child to nullptr if are_isomorphic is false. This will ensure we won't use the value by mistake (since it's not meaningful in that case). Simon