[PATCH 0/3] [gdb/symtab] Eliminate deferred_entry

[PATCH 0/3] [gdb/symtab] Eliminate deferred_entry

[Tom de Vries]

In a discussion of a proposed fix for PR symtab/30728 (
https://sourceware.org/pipermail/gdb-patches/2023-December/205068.html ), this
was mentioned:
...
I wonder then about just sticking this info directly into the
cooked_index_entry object, and then doing fixups directly on these in
the shard.

That is, instead of keeping separate "deferred" entries, just making
ordinary entries.  cooked_index_entry::parent_entry could be a union
holding either the parent (if known) or a CORE_ADDR; and then there
could be a new flag in cooked_index_flag_enum indicating which one is in
use.
...

This patch series implements that idea.

Tested on x86_64-linux.

Tom de Vries (3):
  [gdb/symtab] Allow changing of added cooked_index entries
  [gdb/symtab] Make cooked_index_entry::parent_entry private
  [gdb/symtab] Eliminate deferred_entry

 gdb/dwarf2/cooked-index.c | 21 ++++++-----
 gdb/dwarf2/cooked-index.h | 78 ++++++++++++++++++++++++++++++++-------
 gdb/dwarf2/read.c         | 51 +++++++++++--------------
 3 files changed, 99 insertions(+), 51 deletions(-)


base-commit: 2bcfdb758bb18e29cf6c0486c448bf78dbbc0c8a
-- 
2.35.3

[PATCH 1/3] [gdb/symtab] Allow changing of added cooked_index entries

[Tom de Vries]

Make cooked_index_storage::add and cooked_index_entry::add return a
"cooked_index_entry *" instead of a "const cooked_index_entry *".

Tested on x86_64-linux.
---
 gdb/dwarf2/cooked-index.c |  2 +-
 gdb/dwarf2/cooked-index.h | 10 +++++-----
 gdb/dwarf2/read.c         | 10 +++++-----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index ba77f9cb373..f9850d39bd0 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -228,7 +228,7 @@ cooked_index_entry::write_scope (struct obstack *storage,
 
 /* See cooked-index.h.  */
 
-const cooked_index_entry *
+cooked_index_entry *
 cooked_index_shard::add (sect_offset die_offset, enum dwarf_tag tag,
 			 cooked_index_flag flags, const char *name,
 			 const cooked_index_entry *parent_entry,
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 3c6b9c35fd5..452a07ffb80 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -257,11 +257,11 @@ class cooked_index_shard
 
   /* Create a new cooked_index_entry and register it with this object.
      Entries are owned by this object.  The new item is returned.  */
-  const cooked_index_entry *add (sect_offset die_offset, enum dwarf_tag tag,
-				 cooked_index_flag flags,
-				 const char *name,
-				 const cooked_index_entry *parent_entry,
-				 dwarf2_per_cu_data *per_cu);
+  cooked_index_entry *add (sect_offset die_offset, enum dwarf_tag tag,
+			   cooked_index_flag flags,
+			   const char *name,
+			   const cooked_index_entry *parent_entry,
+			   dwarf2_per_cu_data *per_cu);
 
   /* Install a new fixed addrmap from the given mutable addrmap.  */
   void install_addrmap (addrmap_mutable *map)
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 6c62932520f..61646a5fba0 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4468,11 +4468,11 @@ class cooked_index_storage
 
   /* Add an entry to the index.  The arguments describe the entry; see
      cooked-index.h.  The new entry is returned.  */
-  const cooked_index_entry *add (sect_offset die_offset, enum dwarf_tag tag,
-				 cooked_index_flag flags,
-				 const char *name,
-				 const cooked_index_entry *parent_entry,
-				 dwarf2_per_cu_data *per_cu)
+  cooked_index_entry *add (sect_offset die_offset, enum dwarf_tag tag,
+			   cooked_index_flag flags,
+			   const char *name,
+			   const cooked_index_entry *parent_entry,
+			   dwarf2_per_cu_data *per_cu)
   {
     return m_index->add (die_offset, tag, flags, name, parent_entry, per_cu);
   }
-- 
2.35.3

[PATCH 2/3] [gdb/symtab] Make cooked_index_entry::parent_entry private

[Tom de Vries]

Make cooked_index_entry::parent_entry private, and add member functions to
access it.

Tested on x86_64-linux.
---
 gdb/dwarf2/cooked-index.c | 14 +++++++-------
 gdb/dwarf2/cooked-index.h | 25 +++++++++++++++++++------
 gdb/dwarf2/read.c         |  6 +++---
 3 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index f9850d39bd0..307cfe74a87 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -186,7 +186,7 @@ cooked_index_entry::full_name (struct obstack *storage, bool for_main) const
 {
   const char *local_name = for_main ? name : canonical;
 
-  if ((flags & IS_LINKAGE) != 0 || parent_entry == nullptr)
+  if ((flags & IS_LINKAGE) != 0 || get_parent () == nullptr)
     return local_name;
 
   const char *sep = nullptr;
@@ -207,7 +207,7 @@ cooked_index_entry::full_name (struct obstack *storage, bool for_main) const
       return local_name;
     }
 
-  parent_entry->write_scope (storage, sep, for_main);
+  get_parent ()->write_scope (storage, sep, for_main);
   obstack_grow0 (storage, local_name, strlen (local_name));
   return (const char *) obstack_finish (storage);
 }
@@ -219,8 +219,8 @@ cooked_index_entry::write_scope (struct obstack *storage,
 				 const char *sep,
 				 bool for_main) const
 {
-  if (parent_entry != nullptr)
-    parent_entry->write_scope (storage, sep, for_main);
+  if (get_parent () != nullptr)
+    get_parent ()->write_scope (storage, sep, for_main);
   const char *local_name = for_main ? name : canonical;
   obstack_grow (storage, local_name, strlen (local_name));
   obstack_grow (storage, sep, strlen (sep));
@@ -299,7 +299,7 @@ cooked_index_shard::handle_gnat_encoded_entry (cooked_index_entry *entry,
       parent = last;
     }
 
-  entry->parent_entry = parent;
+  entry->set_parent (parent);
   return make_unique_xstrndup (tail.data (), tail.length ());
 }
 
@@ -585,9 +585,9 @@ cooked_index::dump (gdbarch *arch) const
       gdb_printf ("    flags:      %s\n", to_string (entry->flags).c_str ());
       gdb_printf ("    DIE offset: %s\n", sect_offset_str (entry->die_offset));
 
-      if (entry->parent_entry != nullptr)
+      if (entry->get_parent () != nullptr)
 	gdb_printf ("    parent:     ((cooked_index_entry *) %p) [%s]\n",
-		    entry->parent_entry, entry->parent_entry->name);
+		    entry->get_parent (), entry->get_parent ()->name);
       else
 	gdb_printf ("    parent:     ((cooked_index_entry *) 0)\n");
 
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 452a07ffb80..2b2f2f79bab 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -84,8 +84,8 @@ struct cooked_index_entry : public allocate_on_obstack
       tag (tag_),
       flags (flags_),
       die_offset (die_offset_),
-      parent_entry (parent_entry_),
-      per_cu (per_cu_)
+      per_cu (per_cu_),
+      m_parent_entry (parent_entry_)
   {
   }
 
@@ -210,6 +210,18 @@ struct cooked_index_entry : public allocate_on_obstack
     return compare (canonical, other.canonical, SORT) < 0;
   }
 
+  /* Set parent entry to PARENT.  */
+  void set_parent (const cooked_index_entry *parent)
+  {
+    m_parent_entry = parent;
+  }
+
+  /* Return parent entry.  */
+  const cooked_index_entry *get_parent () const
+  {
+    return m_parent_entry;
+  }
+
   /* The name as it appears in DWARF.  This always points into one of
      the mapped DWARF sections.  Note that this may be the name or the
      linkage name -- two entries are created for DIEs which have both
@@ -224,10 +236,6 @@ struct cooked_index_entry : public allocate_on_obstack
   cooked_index_flag flags;
   /* The offset of this DIE.  */
   sect_offset die_offset;
-  /* The parent entry.  This is NULL for top-level entries.
-     Otherwise, it points to the parent entry, such as a namespace or
-     class.  */
-  const cooked_index_entry *parent_entry;
   /* The CU from which this entry originates.  */
   dwarf2_per_cu_data *per_cu;
 
@@ -238,6 +246,11 @@ struct cooked_index_entry : public allocate_on_obstack
      a parent, its write_scope method is called first.  */
   void write_scope (struct obstack *storage, const char *sep,
 		    bool for_name) const;
+
+  /* The parent entry.  This is NULL for top-level entries.
+     Otherwise, it points to the parent entry, such as a namespace or
+     class.  */
+  const cooked_index_entry *m_parent_entry;
 };
 
 class cooked_index;
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 61646a5fba0..841746b7473 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16421,7 +16421,7 @@ cooked_indexer::index_dies (cutu_reader *reader,
       /* The scope of a DW_TAG_entry_point cooked_index_entry is the one of
 	 its surrounding subroutine.  */
       if (abbrev->tag == DW_TAG_entry_point)
-	this_parent_entry = parent_entry->parent_entry;
+	this_parent_entry = parent_entry->get_parent ();
       info_ptr = scan_attributes (reader->cu->per_cu, reader, info_ptr,
 				  info_ptr, abbrev, &name, &linkage_name,
 				  &flags, &sibling, &this_parent_entry,
@@ -16764,7 +16764,7 @@ cooked_index_functions::expand_symtabs_matching
 	     matches.  */
 	  bool found = true;
 
-	  const cooked_index_entry *parent = entry->parent_entry;
+	  const cooked_index_entry *parent = entry->get_parent ();
 	  for (int i = name_vec.size () - 1; i > 0; --i)
 	    {
 	      /* If we ran out of entries, or if this segment doesn't
@@ -16777,7 +16777,7 @@ cooked_index_functions::expand_symtabs_matching
 		  break;
 		}
 
-	      parent = parent->parent_entry;
+	      parent = parent->get_parent ();
 	    }
 
 	  if (!found)
-- 
2.35.3

[PATCH 3/3] [gdb/symtab] Eliminate deferred_entry

[Tom de Vries]

Currently cooked_index entry creation is either:
- done immediately if the parent_entry is known, or
- deferred if the parent_entry is not yet known, and done later while
  resolving the deferred entries.

Instead, create all cooked_index entries immediately, and keep track of which
entries have a parent_entry that needs resolving later using the new
IS_PARENT_DEFERRED flag.

Tested on x86_64-linux.
---
 gdb/dwarf2/cooked-index.c |  7 ++++--
 gdb/dwarf2/cooked-index.h | 51 ++++++++++++++++++++++++++++++++++-----
 gdb/dwarf2/read.c         | 37 ++++++++++++----------------
 3 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 307cfe74a87..05d277bf215 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -231,7 +231,7 @@ cooked_index_entry::write_scope (struct obstack *storage,
 cooked_index_entry *
 cooked_index_shard::add (sect_offset die_offset, enum dwarf_tag tag,
 			 cooked_index_flag flags, const char *name,
-			 const cooked_index_entry *parent_entry,
+			 cooked_index_entry_ref parent_entry,
 			 dwarf2_per_cu_data *per_cu)
 {
   cooked_index_entry *result = create (die_offset, tag, flags, name,
@@ -585,7 +585,10 @@ cooked_index::dump (gdbarch *arch) const
       gdb_printf ("    flags:      %s\n", to_string (entry->flags).c_str ());
       gdb_printf ("    DIE offset: %s\n", sect_offset_str (entry->die_offset));
 
-      if (entry->get_parent () != nullptr)
+      if ((entry->flags & IS_PARENT_DEFERRED) != 0)
+	gdb_printf ("    parent:     deferred (%" PRIx64 ")\n",
+		    entry->get_deferred_parent ());
+      else if (entry->get_parent () != nullptr)
 	gdb_printf ("    parent:     ((cooked_index_entry *) %p) [%s]\n",
 		    entry->get_parent (), entry->get_parent ()->name);
       else
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 2b2f2f79bab..1bfad27f263 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -38,6 +38,7 @@
 struct dwarf2_per_cu_data;
 struct dwarf2_per_bfd;
 struct index_cache_store_context;
+struct cooked_index_entry;
 
 /* Flags that describe an entry in the index.  */
 enum cooked_index_flag_enum : unsigned char
@@ -53,9 +54,30 @@ enum cooked_index_flag_enum : unsigned char
   /* True if this entry is just for the declaration of a type, not the
      definition.  */
   IS_TYPE_DECLARATION = 16,
+  /* True is parent_entry.deferred has a value rather than parent_entry
+     .resolved.  */
+  IS_PARENT_DEFERRED = 32,
 };
 DEF_ENUM_FLAGS_TYPE (enum cooked_index_flag_enum, cooked_index_flag);
 
+/* Type representing either a resolved or deferred cooked_index_entry.  */
+
+union cooked_index_entry_ref
+{
+  cooked_index_entry_ref (CORE_ADDR deferred_)
+  {
+    deferred = deferred_;
+  }
+
+  cooked_index_entry_ref (const cooked_index_entry *resolved_)
+  {
+    resolved = resolved_;
+  }
+
+  const cooked_index_entry *resolved;
+  CORE_ADDR deferred;
+};
+
 /* Return a string representation of FLAGS.  */
 
 std::string to_string (cooked_index_flag flags);
@@ -78,7 +100,7 @@ struct cooked_index_entry : public allocate_on_obstack
 {
   cooked_index_entry (sect_offset die_offset_, enum dwarf_tag tag_,
 		      cooked_index_flag flags_, const char *name_,
-		      const cooked_index_entry *parent_entry_,
+		      cooked_index_entry_ref parent_entry_,
 		      dwarf2_per_cu_data *per_cu_)
     : name (name_),
       tag (tag_),
@@ -213,13 +235,30 @@ struct cooked_index_entry : public allocate_on_obstack
   /* Set parent entry to PARENT.  */
   void set_parent (const cooked_index_entry *parent)
   {
-    m_parent_entry = parent;
+    gdb_assert ((flags & IS_PARENT_DEFERRED) == 0);
+    m_parent_entry.resolved = parent;
+  }
+
+  /* Resolve deferred parent entry to PARENT.  */
+  void resolve_parent (const cooked_index_entry *parent)
+  {
+    gdb_assert ((flags & IS_PARENT_DEFERRED) != 0);
+    flags = flags & ~IS_PARENT_DEFERRED;
+    m_parent_entry.resolved = parent;
   }
 
   /* Return parent entry.  */
   const cooked_index_entry *get_parent () const
   {
-    return m_parent_entry;
+    gdb_assert ((flags & IS_PARENT_DEFERRED) == 0);
+    return m_parent_entry.resolved;
+  }
+
+  /* Return deferred parent entry.  */
+  CORE_ADDR get_deferred_parent () const
+  {
+    gdb_assert ((flags & IS_PARENT_DEFERRED) != 0);
+    return m_parent_entry.deferred;
   }
 
   /* The name as it appears in DWARF.  This always points into one of
@@ -250,7 +289,7 @@ struct cooked_index_entry : public allocate_on_obstack
   /* The parent entry.  This is NULL for top-level entries.
      Otherwise, it points to the parent entry, such as a namespace or
      class.  */
-  const cooked_index_entry *m_parent_entry;
+  cooked_index_entry_ref m_parent_entry;
 };
 
 class cooked_index;
@@ -273,7 +312,7 @@ class cooked_index_shard
   cooked_index_entry *add (sect_offset die_offset, enum dwarf_tag tag,
 			   cooked_index_flag flags,
 			   const char *name,
-			   const cooked_index_entry *parent_entry,
+			   cooked_index_entry_ref parent_entry,
 			   dwarf2_per_cu_data *per_cu);
 
   /* Install a new fixed addrmap from the given mutable addrmap.  */
@@ -332,7 +371,7 @@ class cooked_index_shard
 			      enum dwarf_tag tag,
 			      cooked_index_flag flags,
 			      const char *name,
-			      const cooked_index_entry *parent_entry,
+			      cooked_index_entry_ref parent_entry,
 			      dwarf2_per_cu_data *per_cu)
   {
     return new (&m_storage) cooked_index_entry (die_offset, tag, flags,
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 841746b7473..c897a222a9f 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4471,7 +4471,7 @@ class cooked_index_storage
   cooked_index_entry *add (sect_offset die_offset, enum dwarf_tag tag,
 			   cooked_index_flag flags,
 			   const char *name,
-			   const cooked_index_entry *parent_entry,
+			   cooked_index_entry_ref parent_entry,
 			   dwarf2_per_cu_data *per_cu)
   {
     return m_index->add (die_offset, tag, flags, name, parent_entry, per_cu);
@@ -4620,16 +4620,6 @@ class cooked_indexer
      understand this.  */
   addrmap_mutable m_die_range_map;
 
-  /* A single deferred entry.  */
-  struct deferred_entry
-  {
-    sect_offset die_offset;
-    const char *name;
-    CORE_ADDR spec_offset;
-    dwarf_tag tag;
-    cooked_index_flag flags;
-  };
-
   /* The generated DWARF can sometimes have the declaration for a
      method in a class (or perhaps namespace) scope, with the
      definition appearing outside this scope... just one of the many
@@ -4637,7 +4627,7 @@ class cooked_indexer
      defer certain entries until the end of scanning, at which point
      we'll know the containing context of all the DIEs that we might
      have scanned.  This vector stores these deferred entries.  */
-  std::vector<deferred_entry> m_deferred_entries;
+  std::vector<cooked_index_entry *> m_deferred_entries;
 };
 
 /* Subroutine of dwarf2_build_psymtabs_hard to simplify it.
@@ -16438,17 +16428,21 @@ cooked_indexer::index_dies (cutu_reader *reader,
 	  name = nullptr;
 	}
 
-      const cooked_index_entry *this_entry = nullptr;
+      cooked_index_entry *this_entry = nullptr;
       if (name != nullptr)
 	{
 	  if (defer != 0)
-	    m_deferred_entries.push_back ({
-		this_die, name, defer, abbrev->tag, flags
-	      });
+	    {
+	      this_entry
+		= m_index_storage->add (this_die, abbrev->tag,
+					flags | IS_PARENT_DEFERRED, name,
+					defer, m_per_cu);
+	      m_deferred_entries.push_back (this_entry);
+	    }
 	  else
-	    this_entry = m_index_storage->add (this_die, abbrev->tag, flags,
-					       name, this_parent_entry,
-					       m_per_cu);
+	    this_entry
+	      = m_index_storage->add (this_die, abbrev->tag, flags, name,
+				      this_parent_entry, m_per_cu);
 	}
 
       if (linkage_name != nullptr)
@@ -16549,10 +16543,9 @@ cooked_indexer::make_index (cutu_reader *reader)
 
   for (const auto &entry : m_deferred_entries)
     {
-      void *obj = m_die_range_map.find (entry.spec_offset);
+      void *obj = m_die_range_map.find (entry->get_deferred_parent ());
       cooked_index_entry *parent = static_cast<cooked_index_entry *> (obj);
-      m_index_storage->add (entry.die_offset, entry.tag, entry.flags,
-			    entry.name, parent, m_per_cu);
+      entry->resolve_parent (parent);
     }
 }
 
-- 
2.35.3

Re: [PATCH 1/3] [gdb/symtab] Allow changing of added cooked_index entries

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

On Thu, Jan 4, 2024 at 3:19 PM Tom de Vries <tdevries@suse.de> wrote:

> Make cooked_index_storage::add and cooked_index_entry::add return a
> "cooked_index_entry *" instead of a "const cooked_index_entry *".
>
> Tested on x86_64-linux.
> ---
>
>  I can confirm this change does not introduce any regressions on Fedora
Rawhide, ppc64le.

Re: [PATCH 2/3] [gdb/symtab] Make cooked_index_entry::parent_entry private

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

On Thu, Jan 4, 2024 at 3:19 PM Tom de Vries <tdevries@suse.de> wrote:

> Make cooked_index_entry::parent_entry private, and add member functions to
> access it.
>
> Tested on x86_64-linux.
> ---
>
>   I can confirm this change does not introduce any regressions on Fedora
> Rawhide, ppc64le.

Re: [PATCH 0/3] [gdb/symtab] Eliminate deferred_entry

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> In a discussion of a proposed fix for PR symtab/30728 (
Tom> https://sourceware.org/pipermail/gdb-patches/2023-December/205068.html ), this
Tom> was mentioned:
Tom> ...
Tom> I wonder then about just sticking this info directly into the
Tom> cooked_index_entry object, and then doing fixups directly on these in
Tom> the shard.

Tom> That is, instead of keeping separate "deferred" entries, just making
Tom> ordinary entries.  cooked_index_entry::parent_entry could be a union
Tom> holding either the parent (if known) or a CORE_ADDR; and then there
Tom> could be a new flag in cooked_index_flag_enum indicating which one is in
Tom> use.
Tom> ...

Tom> This patch series implements that idea.

Thank you.  This is ok.
Approved-By: Tom Tromey <tom@tromey.com>

Tom