[PATCH v2 0/4] Fix .gdb_index with Ada

[PATCH v2 0/4] Fix .gdb_index with Ada

[Tom Tromey]

This is v2 of my series to fix .gdb_index support with Ada.

I believe this fixes the issues pointed out by Tom de Vries in the
last version -- in particular, c-linkage-name.exp passes now.

This version also adds another patch to remove type linkage names from
the cooked index (and thus also .gdb_index).  I believe when I wrote
the new reader, I thought it was necessary to preserve these -- but
since they aren't demangled anywhere, this never had any effect
anyway.

I regression tested this on x86-64 Fedora 34, using both the default
and the cc-with-gdb-index board.

Tom

[PATCH v2 1/4] Fix regression in c-linkage-name.exp with gdb index

[Tom Tromey]

c-linkage-name.exp started failing with the gdb-index target board due
to an earlier patch.  The problem here is that some linkage names must
be in the index -- but, based on inspection, not C++ linkage names.
This patch updates the code to exclude only these.
---
 gdb/dwarf2/index-write.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 4827b594994..4f89dfb65f7 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1111,12 +1111,13 @@ write_cooked_index (cooked_index_vector *table,
 
   for (const cooked_index_entry *entry : table->all_entries ())
     {
-      /* GDB never put linkage names into .gdb_index.  The theory here
-	 is that a linkage name will normally be in the minimal
+      /* GDB never put C++ linkage names into .gdb_index.  The theory
+	 here is that a linkage name will normally be in the minimal
 	 symbols anyway, so including it in the index is usually
 	 redundant -- and the cases where it would not be redundant
 	 are rare and not worth supporting.  */
-      if ((entry->flags & IS_LINKAGE) != 0)
+      if (entry->per_cu->lang () == language_cplus
+	  && (entry->flags & IS_LINKAGE) != 0)
 	continue;
 
       const auto it = cu_index_htab.find (entry->per_cu);
-- 
2.34.3

[PATCH v2 2/4] Don't add type linkage names to cooked index

[Tom Tromey]

The compiler will sometimes emit a linkage name for a type, like:

    <1d3>   DW_AT_linkage_name: (indirect string, offset: 0x106f): 11__mbstate_t

These names aren't very useful, and this patch changes the DWARF
reader so that they are ignored by the cooked index.
---
 gdb/dwarf2/read.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 78f4cc1f60d..96069545000 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -17991,11 +17991,6 @@ tag_can_have_linkage_name (enum dwarf_tag tag)
 {
   switch (tag)
     {
-      /* We include types here because an anonymous C++ type might
-	 have a name for linkage purposes.  */
-    case DW_TAG_class_type:
-    case DW_TAG_structure_type:
-    case DW_TAG_union_type:
     case DW_TAG_variable:
     case DW_TAG_subprogram:
       return true;
-- 
2.34.3

[PATCH v2 3/4] Improve Ada support in .gdb_index

[Tom Tromey]

The cooked index work changed how .gdb_index is constructed, and in
the process broke .gdb_index support.  This is PR symtab/29179.

This patch partially fixes the problem.  It arranges for Ada names to
be encoded in the form expected by the index code.  In particular,
linkage names for Ada are emitted, including the "main" name; names
are Ada-encoded; and names are no longer case-folded, something that
prevented operator names from round-tripping correctly.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29179
---
 gdb/ada-lang.c           |  8 +++++---
 gdb/ada-lang.h           |  2 +-
 gdb/dwarf2/index-write.c | 44 ++++++++++++++++++++++++++++++++--------
 3 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 0e3f0daa416..ff9beb47cfd 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -1146,12 +1146,14 @@ ada_fold_name (gdb::string_view name, bool throw_on_error = false)
   return fold_storage.c_str ();
 }
 
-/* The "encoded" form of DECODED, according to GNAT conventions.  */
+/* The "encoded" form of DECODED, according to GNAT conventions.  If
+   FOLD is true (the default), case-fold any ordinary symbol.  Symbols
+   with <...> quoting are not folded in any case.  */
 
 std::string
-ada_encode (const char *decoded)
+ada_encode (const char *decoded, bool fold)
 {
-  if (decoded[0] != '<')
+  if (fold && decoded[0] != '<')
     decoded = ada_fold_name (decoded);
   return ada_encode_1 (decoded, true);
 }
diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index df648c2297e..84c691b94ee 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -316,7 +316,7 @@ extern struct type *ada_get_base_type (struct type *);
 
 extern struct type *ada_check_typedef (struct type *);
 
-extern std::string ada_encode (const char *);
+extern std::string ada_encode (const char *, bool fold = true);
 
 extern const char *ada_enum_name (const char *);
 
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 4f89dfb65f7..6b4052c3467 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1109,22 +1109,48 @@ write_cooked_index (cooked_index_vector *table,
   htab_up var_names (htab_create_alloc (10, htab_hash_string, htab_eq_string,
 					nullptr, xcalloc, xfree));
 
+  const char *main_for_ada = main_name ();
+
   for (const cooked_index_entry *entry : table->all_entries ())
     {
-      /* GDB never put C++ linkage names into .gdb_index.  The theory
-	 here is that a linkage name will normally be in the minimal
-	 symbols anyway, so including it in the index is usually
-	 redundant -- and the cases where it would not be redundant
-	 are rare and not worth supporting.  */
-      if (entry->per_cu->lang () == language_cplus
-	  && (entry->flags & IS_LINKAGE) != 0)
-	continue;
-
       const auto it = cu_index_htab.find (entry->per_cu);
       gdb_assert (it != cu_index_htab.cend ());
 
       const char *name = entry->full_name (&symtab->m_string_obstack);
 
+      if (entry->per_cu->lang () == language_ada)
+	{
+	  /* We want to ensure that the Ada main function's name
+	     appears verbatim in the index.  However, this name will
+	     be of the form "_ada_mumble", and will be rewritten by
+	     ada_decode.  So, recognize it specially here and add it
+	     to the index by hand.  */
+	  if (entry->tag == DW_TAG_subprogram
+	      && strcmp (main_for_ada, name) == 0)
+	    {
+	      /* Leave it alone.  */
+	    }
+	  else
+	    {
+	      /* In order for the index to work when read back into
+		 gdb, it has to use the encoded name, with any
+		 suffixes stripped.  */
+	      std::string encoded = ada_encode (name, false);
+	      name = obstack_strdup (&symtab->m_string_obstack,
+				     encoded.c_str ());
+	    }
+	}
+      else if (entry->per_cu->lang () == language_cplus
+	       && (entry->flags & IS_LINKAGE) != 0)
+	{
+	  /* GDB never put C++ linkage names into .gdb_index.  The
+	     theory here is that a linkage name will normally be in
+	     the minimal symbols anyway, so including it in the index
+	     is usually redundant -- and the cases where it would not
+	     be redundant are rare and not worth supporting.  */
+	  continue;
+	}
+
       gdb_index_symbol_kind kind;
       if (entry->tag == DW_TAG_subprogram)
 	kind = GDB_INDEX_SYMBOL_KIND_FUNCTION;
-- 
2.34.3

[PATCH v2 4/4] Change .gdb_index de-duplication implementation

[Tom Tromey]

While investigating PR symtab/29179, I found that one Ada test failed
because, although a certain symbol was present in the index, with the
new DWARF reader it pointed to a different CU than was chosen by
earlier versions of gdb.

This patch changes how symbol de-duplication is done, deferring the
process until the entire symbol table has been constructed.  This way,
it's possible to always choose the lower-numbered CU among duplicates,
which is how gdb (implicitly) previously worked.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29179
---
 gdb/dwarf2/index-write.c | 77 +++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 36 deletions(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 6b4052c3467..f592734addc 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -176,6 +176,10 @@ struct symtab_index_entry
   /* A sorted vector of the indices of all the CUs that hold an object
      of this name.  */
   std::vector<offset_type> cu_indices;
+
+  /* Minimize CU_INDICES, sorting them and removing duplicates as
+     appropriate.  */
+  void minimize ();
 };
 
 /* The symbol table.  This is a power-of-2-sized hash table.  */
@@ -186,6 +190,13 @@ struct mapped_symtab
     data.resize (1024);
   }
 
+  /* Minimize each entry in the symbol table, removing duplicates.  */
+  void minimize ()
+  {
+    for (symtab_index_entry &item : data)
+      item.minimize ();
+  }
+
   offset_type n_elements = 0;
   std::vector<symtab_index_entry> data;
 
@@ -271,21 +282,36 @@ add_index_entry (struct mapped_symtab *symtab, const char *name,
   slot.cu_indices.push_back (cu_index_and_attrs);
 }
 
-/* Sort and remove duplicates of all symbols' cu_indices lists.  */
+/* See symtab_index_entry.  */
 
-static void
-uniquify_cu_indices (struct mapped_symtab *symtab)
+void
+symtab_index_entry::minimize ()
 {
-  for (auto &entry : symtab->data)
+  if (name == nullptr || cu_indices.empty ())
+    return;
+
+  std::sort (cu_indices.begin (), cu_indices.end ());
+  auto from = std::unique (cu_indices.begin (), cu_indices.end ());
+  cu_indices.erase (from, cu_indices.end ());
+
+  /* We don't want to enter a variable or type more than once, so
+     remove any such duplicates from the list as well.  When doing
+     this, we want to keep the entry from the first CU -- but this is
+     implicit due to the sort.  This choice is done because it's
+     similar to what gdb historically did for partial symbols.  */
+  std::unordered_set<offset_type> seen;
+  from = std::remove_if (cu_indices.begin (), cu_indices.end (),
+			 [&] (offset_type val)
     {
-      if (entry.name != NULL && !entry.cu_indices.empty ())
-	{
-	  auto &cu_indices = entry.cu_indices;
-	  std::sort (cu_indices.begin (), cu_indices.end ());
-	  auto from = std::unique (cu_indices.begin (), cu_indices.end ());
-	  cu_indices.erase (from, cu_indices.end ());
-	}
-    }
+      gdb_index_symbol_kind kind = GDB_INDEX_SYMBOL_KIND_VALUE (val);
+      if (kind != GDB_INDEX_SYMBOL_KIND_TYPE
+	  && kind != GDB_INDEX_SYMBOL_KIND_VARIABLE)
+	return false;
+
+      val &= ~GDB_INDEX_CU_MASK;
+      return !seen.insert (val).second;
+    });
+  cu_indices.erase (from, cu_indices.end ());
 }
 
 /* A form of 'const char *' suitable for container keys.  Only the
@@ -1100,15 +1126,6 @@ write_cooked_index (cooked_index_vector *table,
 		    const cu_index_map &cu_index_htab,
 		    struct mapped_symtab *symtab)
 {
-  /* We track type names and only enter a given type once.  */
-  htab_up type_names (htab_create_alloc (10, htab_hash_string, htab_eq_string,
-					 nullptr, xcalloc, xfree));
-  /* Same with variable names.  However, if a type and variable share
-     a name, we want both, which is why there are two hash tables
-     here.  */
-  htab_up var_names (htab_create_alloc (10, htab_hash_string, htab_eq_string,
-					nullptr, xcalloc, xfree));
-
   const char *main_for_ada = main_name ();
 
   for (const cooked_index_entry *entry : table->all_entries ())
@@ -1157,24 +1174,12 @@ write_cooked_index (cooked_index_vector *table,
       else if (entry->tag == DW_TAG_variable
 	       || entry->tag == DW_TAG_constant
 	       || entry->tag == DW_TAG_enumerator)
-	{
-	  kind = GDB_INDEX_SYMBOL_KIND_VARIABLE;
-	  void **slot = htab_find_slot (var_names.get (), name, INSERT);
-	  if (*slot != nullptr)
-	    continue;
-	  *slot = (void *) name;
-	}
+	kind = GDB_INDEX_SYMBOL_KIND_VARIABLE;
       else if (entry->tag == DW_TAG_module
 	       || entry->tag == DW_TAG_common_block)
 	kind = GDB_INDEX_SYMBOL_KIND_OTHER;
       else
-	{
-	  kind = GDB_INDEX_SYMBOL_KIND_TYPE;
-	  void **slot = htab_find_slot (type_names.get (), name, INSERT);
-	  if (*slot != nullptr)
-	    continue;
-	  *slot = (void *) name;
-	}
+	kind = GDB_INDEX_SYMBOL_KIND_TYPE;
 
       add_index_entry (symtab, name, (entry->flags & IS_STATIC) != 0,
 		       kind, it->second);
@@ -1252,7 +1257,7 @@ write_gdbindex (dwarf2_per_objfile *per_objfile,
 
   /* Now that we've processed all symbols we can shrink their cu_indices
      lists.  */
-  uniquify_cu_indices (&symtab);
+  symtab.minimize ();
 
   data_buf symtab_vec, constant_pool;
   if (symtab.n_elements == 0)
-- 
2.34.3

Re: [PATCH v2 0/4] Fix .gdb_index with Ada

[Tom de Vries]

On 10/14/22 18:08, Tom Tromey via Gdb-patches wrote:
> This is v2 of my series to fix .gdb_index support with Ada.
> 
> I believe this fixes the issues pointed out by Tom de Vries in the
> last version -- in particular, c-linkage-name.exp passes now.
> 
> This version also adds another patch to remove type linkage names from
> the cooked index (and thus also .gdb_index).  I believe when I wrote
> the new reader, I thought it was necessary to preserve these -- but
> since they aren't demangled anywhere, this never had any effect
> anyway.
> 
> I regression tested this on x86-64 Fedora 34, using both the default
> and the cc-with-gdb-index board.


Hi,

I've tested this on openSUSE Leap 15.4 and openSUSE Tumbleweed.

All is looking good apart from the 12.1 regression I found on Tumbleweed 
( https://sourceware.org/bugzilla/show_bug.cgi?id=29694 ), which this 
series doesn't fix for me.

Thanks,
- Tom

Re: [PATCH v2 0/4] Fix .gdb_index with Ada

[Tom Tromey]

Tom> All is looking good apart from the 12.1 regression I found on
Tom> Tumbleweed ( https://sourceware.org/bugzilla/show_bug.cgi?id=29694 ),
Tom> which this series doesn't fix for me.

I'm going to check this series in.  That bug seems to be independent,
and I am working on a fix.

Tom