Re: [PATCH][gdb] Fix missing symtab includes

[Simon Marchi <simark@simark.ca>]

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 <simon.marchi@polymtl.ca>
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