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

[PATCH v2 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.

Submission history:

v2:
- rebase after "Index DWARF in the background" changes

v1:
- https://sourceware.org/pipermail/gdb-patches/2024-January/205666.html
- approved by Tom Tromey

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 | 24 ++++++-----
 gdb/dwarf2/cooked-index.h | 88 +++++++++++++++++++++++++++++++--------
 gdb/dwarf2/read.c         | 41 ++++++++----------
 3 files changed, 101 insertions(+), 52 deletions(-)


base-commit: d006ec41c448d9f4a84df50391e87cbf0aa8c152
-- 
2.35.3

[PATCH v2 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 and ppc64le-linux.
Tested-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
---
 gdb/dwarf2/cooked-index.c |  2 +-
 gdb/dwarf2/cooked-index.h | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 661edf5f06e..c82f49e1462 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -245,7 +245,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 b14930d558f..a2c0d3c23b5 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -267,11 +267,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)
@@ -383,11 +383,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 v2 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 and ppc64le-linux.
Tested-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
---
 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 c82f49e1462..a4a400e8eae 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -203,7 +203,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;
@@ -224,7 +224,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);
 }
@@ -236,8 +236,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));
@@ -310,7 +310,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 ());
 }
 
@@ -638,9 +638,9 @@ cooked_index::dump (gdbarch *arch)
       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 a2c0d3c23b5..0b6e59333da 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -94,8 +94,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_)
   {
   }
 
@@ -220,6 +220,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
@@ -234,10 +246,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;
 
@@ -248,6 +256,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 e055371dc6a..818e0f6e58f 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16508,7 +16508,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,
@@ -16847,7 +16847,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
@@ -16860,7 +16860,7 @@ cooked_index_functions::expand_symtabs_matching
 		  break;
 		}
 
-	      parent = parent->parent_entry;
+	      parent = parent->get_parent ();
 	    }
 
 	  if (!found)
-- 
2.35.3

[PATCH v2 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 | 10 +++++---
 gdb/dwarf2/cooked-index.h | 53 +++++++++++++++++++++++++++++++++------
 gdb/dwarf2/read.c         | 35 +++++++++++---------------
 3 files changed, 67 insertions(+), 31 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index a4a400e8eae..790df66d948 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -248,7 +248,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,
@@ -259,7 +259,8 @@ cooked_index_shard::add (sect_offset die_offset, enum dwarf_tag tag,
      implicit "main" discovery.  */
   if ((flags & IS_MAIN) != 0)
     m_main = result;
-  else if (parent_entry == nullptr
+  else if ((flags & IS_PARENT_DEFERRED) == 0
+	   && parent_entry.resolved == nullptr
 	   && m_main == nullptr
 	   && language_may_use_plain_main (per_cu->lang ())
 	   && strcmp (name, "main") == 0)
@@ -638,7 +639,10 @@ cooked_index::dump (gdbarch *arch)
       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 0b6e59333da..2f579210da8 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -48,6 +48,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
@@ -63,9 +64,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);
@@ -88,7 +110,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_),
@@ -223,13 +245,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
@@ -260,7 +299,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;
@@ -283,7 +322,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.  */
@@ -334,7 +373,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,
@@ -399,7 +438,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);
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 818e0f6e58f..25b53d622a0 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4552,16 +4552,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
@@ -4569,7 +4559,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.
@@ -16525,17 +16515,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)
@@ -16636,10 +16630,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 v2 0/3] [gdb/symtab] Eliminate deferred_entry

[Tom Tromey]

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

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 looks good.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

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

[Tom de Vries]

On 1/9/24 18:04, Tom de Vries wrote:
> @@ -63,9 +64,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);

Pushed this series, with this missing bit added:
...
@@ -51,6 +51,7 @@ to_string (cooked_index_flag flags)
      MAP_ENUM_FLAG (IS_ENUM_CLASS),
      MAP_ENUM_FLAG (IS_LINKAGE),
      MAP_ENUM_FLAG (IS_TYPE_DECLARATION),
+    MAP_ENUM_FLAG (IS_PARENT_DEFERRED),
    };

    return flags.to_string (mapping);
...

Thanks,
- Tom