From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 60EF4385B835 for ; Sun, 29 Mar 2020 21:44:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 60EF4385B835 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (unknown [192.222.164.54]) (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 9C2181E5F8; Sun, 29 Mar 2020 17:44:16 -0400 (EDT) Subject: Re: [PATCH][gdb] Fix missing symtab includes To: Tom de Vries , gdb-patches@sourceware.org Cc: Tom Tromey References: <20200327204948.GA23365@delia> <44300932-0e15-c3b6-9ce8-44e08dd5d8ef@suse.de> From: Simon Marchi Message-ID: Date: Sun, 29 Mar 2020 17:44:15 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US-large Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-25.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, 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: Sun, 29 Mar 2020 21:44:23 -0000 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. As you probably saw, I renamed it to expand_dependencies to be more accurate (since it calls expand, not read). 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. Here's the version I came up with, if you want to get inspiration. >From ec02d85b88d6a9bb842c69c0630e3e944357e4d3 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Sun, 29 Mar 2020 14:21:16 -0400 Subject: [PATCH] gdb: make dwarf2_include_psymtab defer to its includer --- gdb/dwarf2/read.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 1ec5c1e582b7..feabaae324d9 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -5831,22 +5831,17 @@ struct dwarf2_include_psymtab : public partial_symtab void read_symtab (struct objfile *objfile) override { - expand_psymtab (objfile); + 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; + gdb_assert (false); } bool readin_p () const override { - return m_readin; + return includer ()->readin_p (); } struct compunit_symtab *get_compunit_symtab () const override @@ -5855,8 +5850,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 @@ -8772,8 +8772,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); -- 2.25.2