From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 11BA9385DC15 for ; Mon, 30 Mar 2020 17:42:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 11BA9385DC15 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id EB06BAFB2; Mon, 30 Mar 2020 17:42:18 +0000 (UTC) Subject: Re: [PATCH][gdb] Fix missing symtab includes To: Simon Marchi , gdb-patches@sourceware.org Cc: Tom Tromey References: <20200327204948.GA23365@delia> <44300932-0e15-c3b6-9ce8-44e08dd5d8ef@suse.de> From: Tom de Vries Autocrypt: addr=tdevries@suse.de; keydata= xsBNBF0ltCcBCADDhsUnMMdEXiHFfqJdXeRvgqSEUxLCy/pHek88ALuFnPTICTwkf4g7uSR7 HvOFUoUyu8oP5mNb4VZHy3Xy8KRZGaQuaOHNhZAT1xaVo6kxjswUi3vYgGJhFMiLuIHdApoc u5f7UbV+egYVxmkvVLSqsVD4pUgHeSoAcIlm3blZ1sDKviJCwaHxDQkVmSsGXImaAU+ViJ5l CwkvyiiIifWD2SoOuFexZyZ7RUddLosgsO0npVUYbl6dEMq2a5ijGF6/rBs1m3nAoIgpXk6P TCKlSWVW6OCneTaKM5C387972qREtiArTakRQIpvDJuiR2soGfdeJ6igGA1FZjU+IsM5ABEB AAHNH1RvbSBkZSBWcmllcyA8dGRldnJpZXNAc3VzZS5kZT7CwKsEEwEIAD4WIQSsnSe5hKbL MK1mGmjuhV2rbOJEoAUCXSW0JwIbAwUJA8JnAAULCQgHAgYVCgkICwIEFgIDAQIeAQIXgAAh CRDuhV2rbOJEoBYhBKydJ7mEpsswrWYaaO6FXats4kSgc48H/Ra2lq5p3dHsrlQLqM7N68Fo eRDf3PMevXyMlrCYDGLVncQwMw3O/AkousktXKQ42DPJh65zoXB22yUt8m0g12xkLax98KFJ 5NyUloa6HflLl+wQL/uZjIdNUQaHQLw3HKwRMVi4l0/Jh/TygYG1Dtm8I4o708JS4y8GQxoQ UL0z1OM9hyM3gI2WVTTyprsBHy2EjMOu/2Xpod95pF8f90zBLajy6qXEnxlcsqreMaqmkzKn 3KTZpWRxNAS/IH3FbGQ+3RpWkNGSJpwfEMVCeyK5a1n7yt1podd1ajY5mA1jcaUmGppqx827 8TqyteNe1B/pbiUt2L/WhnTgW1NC1QDOwE0EXSW0JwEIAM99H34Bu4MKM7HDJVt864MXbx7B 1M93wVlpJ7Uq+XDFD0A0hIal028j+h6jA6bhzWto4RUfDl/9mn1StngNVFovvwtfzbamp6+W pKHZm9X5YvlIwCx131kTxCNDcF+/adRW4n8CU3pZWYmNVqhMUiPLxElA6QhXTtVBh1RkjCZQ Kmbd1szvcOfaD8s+tJABJzNZsmO2hVuFwkDrRN8Jgrh92a+yHQPd9+RybW2l7sJv26nkUH5Z 5s84P6894ebgimcprJdAkjJTgprl1nhgvptU5M9Uv85Pferoh2groQEAtRPlCGrZ2/2qVNe9 XJfSYbiyedvApWcJs5DOByTaKkcAEQEAAcLAkwQYAQgAJhYhBKydJ7mEpsswrWYaaO6FXats 4kSgBQJdJbQnAhsMBQkDwmcAACEJEO6FXats4kSgFiEErJ0nuYSmyzCtZhpo7oVdq2ziRKD3 twf7BAQBZ8TqR812zKAD7biOnWIJ0McV72PFBxmLIHp24UVe0ZogtYMxSWKLg3csh0yLVwc7 H3vldzJ9AoK3Qxp0Q6K/rDOeUy3HMqewQGcqrsRRh0NXDIQk5CgSrZslPe47qIbe3O7ik/MC q31FNIAQJPmKXX25B115MMzkSKlv4udfx7KdyxHrTSkwWZArLQiEZj5KG4cCKhIoMygPTA3U yGaIvI/BGOtHZ7bEBVUCFDFfOWJ26IOCoPnSVUvKPEOH9dv+sNy7jyBsP5QxeTqwxC/1ZtNS DUCSFQjqA6bEGwM22dP8OUY6SC94x1G81A9/xbtm9LQxKm0EiDH8KBMLfQ== Message-ID: <93eaa135-a031-e3cf-9fd5-293b3fc437fe@suse.de> Date: Mon, 30 Mar 2020 19:42:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------CF3185E8F95BAA3BCEF97520" Content-Language: en-US X-Spam-Status: No, score=-33.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, KAM_SHORT, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, 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, 30 Mar 2020 17:42:22 -0000 This is a multi-part message in MIME format. --------------CF3185E8F95BAA3BCEF97520 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 29-03-2020 23:44, Simon Marchi wrote: > On 2020-03-29 11:56 a.m., Tom de Vries wrote: >> diff --git a/gdb/psympriv.h b/gdb/psympriv.h >> index 0effedc4ec..4317392149 100644 >> --- a/gdb/psympriv.h >> +++ b/gdb/psympriv.h >> @@ -124,16 +124,26 @@ struct partial_symtab >> { >> } >> >> - /* Read the full symbol table corresponding to this partial symbol >> - table. */ >> + /* Psymtab expansion is done in two steps: >> + - a call to read_symtab >> + - while that call is in progress, calls to expand_psymtab can be made, >> + both for this psymtab, and its dependencies. >> + This makes a distinction between a toplevel psymtab (for which both >> + read_symtab and expand_psymtab will be called) and a non-toplevel >> + psymtab (for which only expand_psymtab will be called). The >> + distinction can be used f.i. to do things before and after all >> + dependencies of a top-level psymtab have been expanded. >> + >> + Read the full symbol table corresponding to this partial symbol >> + table. Typically calls expand_psymtab. */ >> virtual void read_symtab (struct objfile *) = 0; >> >> - /* Psymtab expansion is done in two steps. The first step is a call >> - to read_symtab; but while that is in progress, calls to >> - expand_psymtab can be made. */ >> + /* Expand the full symbol table for this partial symbol table. Typically >> + calls read_dependencies. */ >> virtual void expand_psymtab (struct objfile *) = 0; >> >> - /* Ensure that all the dependencies are read in. */ >> + /* Ensure that all the dependencies are read in. Typically calls >> + expand_psymtab for each dependency. */ >> void read_dependencies (struct objfile *); > > I don't think the "Typically" here is right. This is not a virtual function that can > be implemented/overriden by sub-classes, it's a helper that sub-classes call, that calls > expand_psymtab on all dependencies. Ack, comment updated accordingly. > As you probably saw, I renamed it to > expand_dependencies to be more accurate (since it calls expand, not read). > Ack, patch rebased. > But otherwise, I am starting to think that it's the right thing to to, to call read_symtab > on the includer (top-level psymtab) in dwarf2_include_psymtab::read_symtab. To read in > an include psymtab, we read in the includer. > > Following this, I also came to the conclusion that dwarf2_include_psymtab::m_readin should > probably be removed. An include psymtab can't be read in on its own. It can be considered > read in when its includer is read in. So it doesn't make sense to maintain a "read in" state > separate from its includer. > That makes sense to me. > Here's the version I came up with, if you want to get inspiration. > I've integrated it into my patch and re-tested. Also, added test-case. Thanks, - Tom --------------CF3185E8F95BAA3BCEF97520 Content-Type: text/x-patch; charset=UTF-8; name="0002-gdb-Fix-missing-symtab-includes.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0002-gdb-Fix-missing-symtab-includes.patch" [gdb] Fix missing symtab includes [ The test-case requires patch "[gdb] Expand symbolless symtabs using maint expand-symtabs" submitted at https://sourceware.org/pipermail/gdb-patches/2020-March/167131.html . ] Consider the debug info for the test-case included in this patch. It consists of a PU: ... <0>: Abbrev Number: 2 (DW_TAG_partial_unit) <1>: Abbrev Number: 0 ... imported by a CU: ... <0>: Abbrev Number: 2 (DW_TAG_compile_unit) DW_AT_language : 2 (non-ANSI C) DW_AT_stmt_list : 0xe9 <1>: Abbrev Number: 3 (DW_TAG_imported_unit) DW_AT_import : <0xd2> [Abbrev Number: 2] <1>: Abbrev Number: 0 ... and the CU has a dw2-symtab-includes.h file in the .debug_line file name table: ... The Directory Table (offset 0x101): 1 /data/gdb_versions/devel/src/gdb/testsuite/gdb.dwarf2 The File Name Table (offset 0x138): Entry Dir Time Size Name 1 1 0 0 dw2-symtab-includes.h ... After expanding all symtabs, we can see the CU listed in the user field of the PU, and vice-versa the PU listed in the includes of the CU: ... $ gdb.sh -batch \ -iex "set language c" \ outputs/gdb.dwarf2/dw2-symtab-includes/dw2-symtab-includes \ -ex "maint expand-symtabs" \ -ex "maint info symtabs" ... { ((struct compunit_symtab *) 0x394dd60) debugformat DWARF 2 producer (null) dirname (null) blockvector ((struct blockvector *) 0x394dea0) user ((struct compunit_symtab *) 0x394dba0) } { ((struct compunit_symtab *) 0x394dba0) debugformat DWARF 2 producer (null) dirname (null) blockvector ((struct blockvector *) 0x394dd10) user ((struct compunit_symtab *) (null)) ( includes ((struct compunit_symtab *) 0x394dd60) ) } ... But if we instead only expand the symtab for the dw2-symtab-includes.h file, the includes and user links are gone: ... $ gdb -batch \ -iex "set language c" \ outputs/gdb.dwarf2/dw2-symtab-includes/dw2-symtab-includes \ -ex "maint expand-symtabs dw2-symtab-includes.h" \ -ex "maint info symtabs" ... { ((struct compunit_symtab *) 0x2728210) debugformat DWARF 2 producer (null) dirname (null) blockvector ((struct blockvector *) 0x2728350) user ((struct compunit_symtab *) (null)) } { ((struct compunit_symtab *) 0x2728050) debugformat DWARF 2 producer (null) dirname (null) blockvector ((struct blockvector *) 0x27281c0) user ((struct compunit_symtab *) (null)) } ... The includes are calculated by process_cu_includes in gdb/dwarf2/read.c. In the case of expanding all symtabs: - the CU partial symtab is expanded using psymtab_to_symtab - psymtab_to_symtab calls dwarf2_psymtab::read_symtab - dwarf2_psymtab::read_symtab calls dwarf2_psymtab::expand_psymtab - dwarf2_psymtab::read_symtab calls process_cu_includes, and we have the includes In the case of expanding the symtab for dw2-symtab-includes.h: - the dw2-symtab-includes.h partial symtab is expanded using psymtab_to_symtab - psymtab_to_symtab calls dwarf2_include_psymtab::read_symtab - dwarf2_include_psymtab::read_symtab calls dwarf2_include_psymtab::expand_psymtab - dwarf2_include_psymtab::expand_psymtab calls partial_symtab::expand_dependencies - partial_symtab::expand_dependencies calls dwarf2_psymtab::expand_psymtab for the CU partial symtab - the CU partial symtab is expanded using dwarf2_psymtab::expand_psymtab - process_cu_includes is never called Fix this by making sure in dwarf2_include_psymtab::read_symtab that read_symtab is called for the CU partial symtab. Tested on x86_64-linux, with native, and target board cc-with-dwz and cc-with-dwz-m. In addition, tested test-case with target boards cc-with-gdb-index.exp, cc-with-debug-names.exp and readnow.exp. gdb/ChangeLog: 2020-03-30 Simon Marchi Tom de Vries PR symtab/25718 * psympriv.h (struct partial_symtab::read_symtab) (struct partial_symtab::expand_psymtab) (struct partial_symtab::read_dependencies): Update comments. * dwarf2/read.c (struct dwarf2_include_psymtab::read_symtab): Call read_symtab for includer. (struct dwarf2_include_psymtab::expand_psymtab): Assert false. (struct dwarf2_include_psymtab::readin_p): Call readin_p () for includer. (struct dwarf2_include_psymtab::m_readin): Remove. (struct dwarf2_include_psymtab::includer): New member function. (dwarf2_psymtab::expand_psymtab): Assert !readin. gdb/testsuite/ChangeLog: 2020-03-30 Tom de Vries PR symtab/25718 * gdb.dwarf2/dw2-symtab-includes.exp: New file. --- gdb/dwarf2/read.c | 37 +++++++---- gdb/psympriv.h | 22 +++++-- gdb/testsuite/gdb.dwarf2/dw2-symtab-includes.exp | 80 ++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 18 deletions(-) diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 673f2c2904..80a6b7f2bf 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -5855,22 +5855,31 @@ struct dwarf2_include_psymtab : public partial_symtab void read_symtab (struct objfile *objfile) override { - expand_psymtab (objfile); + /* It's an include file, no symbols to read for it. + Everything is in the includer symtab. */ + + /* The expansion of a dwarf2_include_psymtab is just a trigger for + expansion of the includer psymtab. We use the dependencies[0] field to + model the includer. But if we go the regular route of calling + expand_psymtab here, and having expand_psymtab call expand_dependencies + to expand the includer, we'll only use expand_psymtab on the includer + (making it a non-toplevel psymtab), while if we expand the includer via + another path, we'll use read_symtab (making it a toplevel psymtab). + So, don't pretend a dwarf2_include_psymtab is an actual toplevel + psymtab, and trigger read_symtab on the includer here directly. */ + includer ()->read_symtab (objfile); } void expand_psymtab (struct objfile *objfile) override { - if (m_readin) - return; - /* It's an include file, no symbols to read for it. - Everything is in the parent symtab. */ - expand_dependencies (objfile); - m_readin = true; + /* This is not called by read_symtab, and should not be called by any + expand_dependencies. */ + gdb_assert (false); } bool readin_p () const override { - return m_readin; + return includer ()->readin_p (); } struct compunit_symtab *get_compunit_symtab () const override @@ -5879,8 +5888,13 @@ struct dwarf2_include_psymtab : public partial_symtab } private: - - bool m_readin = false; + partial_symtab *includer () const + { + /* An include psymtab has exactly one dependency: the psymtab that + includes it. */ + gdb_assert (this->number_of_dependencies == 1); + return this->dependencies[0]; + } }; /* Allocate a new partial symtab for file named NAME and mark this new @@ -8796,8 +8810,7 @@ process_queue (struct dwarf2_per_objfile *dwarf2_per_objfile) void dwarf2_psymtab::expand_psymtab (struct objfile *objfile) { - if (readin) - return; + gdb_assert (!readin); expand_dependencies (objfile); diff --git a/gdb/psympriv.h b/gdb/psympriv.h index 9bc960a77d..fdcee99e33 100644 --- a/gdb/psympriv.h +++ b/gdb/psympriv.h @@ -124,16 +124,26 @@ struct partial_symtab { } - /* Read the full symbol table corresponding to this partial symbol - table. */ + /* Psymtab expansion is done in two steps: + - a call to read_symtab + - while that call is in progress, calls to expand_psymtab can be made, + both for this psymtab, and its dependencies. + This makes a distinction between a toplevel psymtab (for which both + read_symtab and expand_psymtab will be called) and a non-toplevel + psymtab (for which only expand_psymtab will be called). The + distinction can be used f.i. to do things before and after all + dependencies of a top-level psymtab have been expanded. + + Read the full symbol table corresponding to this partial symbol + table. Typically calls expand_psymtab. */ virtual void read_symtab (struct objfile *) = 0; - /* Psymtab expansion is done in two steps. The first step is a call - to read_symtab; but while that is in progress, calls to - expand_psymtab can be made. */ + /* Expand the full symbol table for this partial symbol table. Typically + calls expand_dependencies. */ virtual void expand_psymtab (struct objfile *) = 0; - /* Ensure that all the dependencies are read in. */ + /* Ensure that all the dependencies are read in. Calls + expand_psymtab for each non-shared dependency. */ void expand_dependencies (struct objfile *); /* Return true if the symtab corresponding to this psymtab has been diff --git a/gdb/testsuite/gdb.dwarf2/dw2-symtab-includes.exp b/gdb/testsuite/gdb.dwarf2/dw2-symtab-includes.exp new file mode 100644 index 0000000000..1eaaf4af4f --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/dw2-symtab-includes.exp @@ -0,0 +1,80 @@ +# Copyright 2020 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# Check that symtab user and includes are present after symtab expansion +# triggered by an include file. + +load_lib dwarf.exp + +# This test can only be run on targets which support DWARF-2 and use gas. +if {![dwarf2_support]} { + return 0 +} + +standard_testfile main.c .S + +# Create the DWARF. +set asm_file [standard_output_file $srcfile2] +Dwarf::assemble $asm_file { + declare_labels partial_label lines_label + global srcdir subdir srcfile + + extern main + + cu {} { + partial_label: partial_unit {} { + } + } + + cu {} { + compile_unit { + {language @DW_LANG_C} + {stmt_list ${lines_label} DW_FORM_sec_offset} + } { + imported_unit { + {import $partial_label ref_addr} + } + } + } + + lines {version 2} lines_label { + include_dir "${srcdir}/${subdir}" + file_name "dw2-symtab-includes.h" 1 + program { + {DW_LNS_advance_line 1} + } + } +} + +if { [prepare_for_testing "failed to prepare" $testfile \ + "${asm_file} ${srcfile}" {}] } { + return -1 +} + +# Check that no symtabs are expanded. +set test "no symtabs expanded" +if { [readnow] } { + unsupported $test + return -1 +} +gdb_test_no_output "maint info symtabs" $test + +# Expand dw2-symtab-includes.h symtab +gdb_test "maint expand-symtab dw2-symtab-includes.h" + +# Check that there are includes. +gdb_test "maint info symtabs" \ + "\r\n \\( includes\r\n.*" \ + "check symtab includes" --------------CF3185E8F95BAA3BCEF97520--