[PATCH 0/4] Some small debug index writer cleanups

[PATCH 0/4] Some small debug index writer cleanups

[Tom Tromey]

I'm working on a patch series that removes the DWARF psymtab reader.
While updating the index writers, I found some small things that would
be convenient to fix for that work, and that could be submitted
separately.  This series is the result.  Let me know what you think.

Tom

[PATCH 1/4] Fix oddity in write_gdbindex

[Tom Tromey]

My recent patch to unify CUs and TUs introduced an oddity in
write_gdbindex.  Here, we pass 'i' to recursively_write_psymbols, but
we must instead pass 'counter', to handle the situation where a TU is
mixed in with the CUs.

I am not sure a test case for this is possible.  I think it can only
happen when using DWARF 5, where a TU appears in .debug_info.
However, this situation is already not handled correctly by
.gdb_index.  I filed a bug about this.

gdb/ChangeLog
2021-05-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/index-write.c (write_gdbindex): Pass 'counter' to
	recursively_write_psymbols.
---
 gdb/ChangeLog            | 5 +++++
 gdb/dwarf2/index-write.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 4b78add0a5a..ffc49e8ba82 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1446,7 +1446,7 @@ write_gdbindex (dwarf2_per_objfile *per_objfile, FILE *out_file,
 	{
 	  if (psymtab->user == NULL)
 	    recursively_write_psymbols (objfile, psymtab, &symtab,
-					psyms_seen, i);
+					psyms_seen, counter);
 
 	  const auto insertpair = cu_index_htab.emplace (psymtab, counter);
 	  gdb_assert (insertpair.second);
-- 
2.26.3

[PATCH 2/4] Minor cleanup to addrmap_index_data::previous_valid

[Tom Tromey]

This changes addrmap_index_data::previous_valid to a bool, and
initializes it inline.

2021-05-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/index-write.c (struct addrmap_index_data)
	<previous_valid>: Now bool.  Initialize.
	(add_address_entry_worker): Update.
	(write_address_map): Update.
---
 gdb/ChangeLog            |  7 +++++++
 gdb/dwarf2/index-write.c | 12 +++---------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index ffc49e8ba82..656742f2e0a 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -422,7 +422,7 @@ struct addrmap_index_data
   /* Non-zero if the previous_* fields are valid.
      We can't write an entry until we see the next entry (since it is only then
      that we know the end of the entry).  */
-  int previous_valid;
+  bool previous_valid = false;
   /* Index of the CU in the table of all CUs in the index file.  */
   unsigned int previous_cu_index;
   /* Start address of the CU.  */
@@ -459,10 +459,10 @@ add_address_entry_worker (void *datap, CORE_ADDR start_addr, void *obj)
       const auto it = data->cu_index_htab.find (pst);
       gdb_assert (it != data->cu_index_htab.cend ());
       data->previous_cu_index = it->second;
-      data->previous_valid = 1;
+      data->previous_valid = true;
     }
   else
-    data->previous_valid = 0;
+    data->previous_valid = false;
 
   return 0;
 }
@@ -477,12 +477,6 @@ write_address_map (dwarf2_per_bfd *per_bfd, data_buf &addr_vec,
 {
   struct addrmap_index_data addrmap_index_data (addr_vec, cu_index_htab);
 
-  /* When writing the address table, we have to cope with the fact that
-     the addrmap iterator only provides the start of a region; we have to
-     wait until the next invocation to get the start of the next region.  */
-
-  addrmap_index_data.previous_valid = 0;
-
   addrmap_foreach (per_bfd->partial_symtabs->psymtabs_addrmap,
 		   add_address_entry_worker, &addrmap_index_data);
 
-- 
2.26.3

[PATCH 3/4] Simplify gdb_index writing

[Tom Tromey]

write_gdbindex write the CUs first, then walks the signatured type
hash table to write out the TUs.  However, now that CUs and TUs are
unified in the DWARF reader, it's simpler to handle both of these in
the same loop.

2021-05-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/index-write.c (write_one_signatured_type): Remove.
	(write_gdbindex): Write type CUs in loop.
---
 gdb/ChangeLog            |  5 +++
 gdb/dwarf2/index-write.c | 84 ++++++++++++----------------------------
 2 files changed, 29 insertions(+), 60 deletions(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 656742f2e0a..06c8e1026f0 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -600,43 +600,6 @@ struct signatured_type_index_data
   int cu_index;
 };
 
-/* A helper function that writes a single signatured_type to an
-   obstack.  */
-
-static int
-write_one_signatured_type (void **slot, void *d)
-{
-  struct signatured_type_index_data *info
-    = (struct signatured_type_index_data *) d;
-  struct signatured_type *entry = (struct signatured_type *) *slot;
-  partial_symtab *psymtab = entry->v.psymtab;
-
-  if (psymtab == nullptr)
-    {
-      /* We can end up here when processing a skeleton CU referring to a
-	 .dwo file that hasn't been found.  There's not much we can do in
-	 such a case, so skip this CU.  */
-      return 1;
-    }
-
-  write_psymbols (info->symtab, info->psyms_seen,
-		  psymtab->global_psymbols, info->cu_index,
-		  0);
-  write_psymbols (info->symtab, info->psyms_seen,
-		  psymtab->static_psymbols, info->cu_index,
-		  1);
-
-  info->types_list.append_uint (8, BFD_ENDIAN_LITTLE,
-				to_underlying (entry->sect_off));
-  info->types_list.append_uint (8, BFD_ENDIAN_LITTLE,
-				to_underlying (entry->type_offset_in_tu));
-  info->types_list.append_uint (8, BFD_ENDIAN_LITTLE, entry->signature);
-
-  ++info->cu_index;
-
-  return 1;
-}
-
 /* Recurse into all "included" dependencies and count their symbols as
    if they appeared in this psymtab.  */
 
@@ -1420,6 +1383,9 @@ write_gdbindex (dwarf2_per_objfile *per_objfile, FILE *out_file,
   psym_index_map cu_index_htab;
   cu_index_htab.reserve (per_objfile->per_bfd->all_comp_units.size ());
 
+  /* Store out the .debug_type CUs, if any.  */
+  data_buf types_cu_list;
+
   /* The CU list is already sorted, so we don't need to do additional
      work here.  Also, the debug_types entries do not appear in
      all_comp_units, but only in their own hash table.  */
@@ -1427,54 +1393,52 @@ write_gdbindex (dwarf2_per_objfile *per_objfile, FILE *out_file,
   std::unordered_set<partial_symbol *> psyms_seen
     (psyms_seen_size (per_objfile));
   int counter = 0;
+  int types_counter = 0;
   for (int i = 0; i < per_objfile->per_bfd->all_comp_units.size (); ++i)
     {
       dwarf2_per_cu_data *per_cu
 	= per_objfile->per_bfd->all_comp_units[i].get ();
-      if (per_cu->is_debug_types)
-	continue;
-
       partial_symtab *psymtab = per_cu->v.psymtab;
 
+      int &this_counter = per_cu->is_debug_types ? types_counter : counter;
+
       if (psymtab != NULL)
 	{
 	  if (psymtab->user == NULL)
 	    recursively_write_psymbols (objfile, psymtab, &symtab,
-					psyms_seen, counter);
+					psyms_seen, this_counter);
 
-	  const auto insertpair = cu_index_htab.emplace (psymtab, counter);
+	  const auto insertpair = cu_index_htab.emplace (psymtab,
+							 this_counter);
 	  gdb_assert (insertpair.second);
 	}
 
       /* The all_comp_units list contains CUs read from the objfile as well as
 	 from the eventual dwz file.  We need to place the entry in the
 	 corresponding index.  */
-      data_buf &cu_list = per_cu->is_dwz ? dwz_cu_list : objfile_cu_list;
+      data_buf &cu_list = (per_cu->is_debug_types
+			   ? types_cu_list
+			   : per_cu->is_dwz ? dwz_cu_list : objfile_cu_list);
       cu_list.append_uint (8, BFD_ENDIAN_LITTLE,
 			   to_underlying (per_cu->sect_off));
-      cu_list.append_uint (8, BFD_ENDIAN_LITTLE, per_cu->length);
-      ++counter;
+      if (per_cu->is_debug_types)
+	{
+	  signatured_type *sig_type = (signatured_type *) per_cu;
+	  cu_list.append_uint (8, BFD_ENDIAN_LITTLE,
+			       to_underlying (sig_type->type_offset_in_tu));
+	  cu_list.append_uint (8, BFD_ENDIAN_LITTLE,
+			       sig_type->signature);
+	}
+      else
+	cu_list.append_uint (8, BFD_ENDIAN_LITTLE, per_cu->length);
+
+      ++this_counter;
     }
 
   /* Dump the address map.  */
   data_buf addr_vec;
   write_address_map (per_objfile->per_bfd, addr_vec, cu_index_htab);
 
-  /* Write out the .debug_type entries, if any.  */
-  data_buf types_cu_list;
-  if (per_objfile->per_bfd->signatured_types)
-    {
-      signatured_type_index_data sig_data (types_cu_list,
-					   psyms_seen);
-
-      sig_data.objfile = objfile;
-      sig_data.symtab = &symtab;
-      sig_data.cu_index = (per_objfile->per_bfd->all_comp_units.size ()
-			   - per_objfile->per_bfd->tu_stats.nr_tus);
-      htab_traverse_noresize (per_objfile->per_bfd->signatured_types.get (),
-			      write_one_signatured_type, &sig_data);
-    }
-
   /* Now that we've processed all symbols we can shrink their cu_indices
      lists.  */
   uniquify_cu_indices (&symtab);
-- 
2.26.3

[PATCH 4/4] Simplify debug_names index writing

[Tom Tromey]

Like the previous patch, this changes the .debug_names writer to find
the TU indices in the main loop over all CUs and TUs.

2021-05-29  Tom Tromey  <tom@tromey.com>

	* dwarf2/index-write.c (struct signatured_type_index_data):
	Remove.
	(debug_names::write_one_signatured_type_data)
	(debug_names::write_one_signatured_type): Remove.
	(write_debug_names): Write type CUs in loop.
---
 gdb/ChangeLog            |  8 ++++
 gdb/dwarf2/index-write.c | 93 +++++-----------------------------------
 2 files changed, 18 insertions(+), 83 deletions(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 06c8e1026f0..6e593310d28 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -585,21 +585,6 @@ write_psymbols (struct mapped_symtab *symtab,
     }
 }
 
-/* A helper struct used when iterating over debug_types.  */
-struct signatured_type_index_data
-{
-  signatured_type_index_data (data_buf &types_list_,
-			      std::unordered_set<partial_symbol *> &psyms_seen_)
-    : types_list (types_list_), psyms_seen (psyms_seen_)
-  {}
-
-  struct objfile *objfile;
-  struct mapped_symtab *symtab;
-  data_buf &types_list;
-  std::unordered_set<partial_symbol *> &psyms_seen;
-  int cu_index;
-};
-
 /* Recurse into all "included" dependencies and count their symbols as
    if they appeared in this psymtab.  */
 
@@ -902,32 +887,6 @@ class debug_names
     m_debugstrlookup.file_write (file_str);
   }
 
-  /* A helper user data for write_one_signatured_type.  */
-  class write_one_signatured_type_data
-  {
-  public:
-    write_one_signatured_type_data (debug_names &nametable_,
-				    signatured_type_index_data &&info_)
-    : nametable (nametable_), info (std::move (info_))
-    {}
-    debug_names &nametable;
-    struct signatured_type_index_data info;
-  };
-
-  /* A helper function to pass write_one_signatured_type to
-     htab_traverse_noresize.  */
-  static int
-  write_one_signatured_type (void **slot, void *d)
-  {
-    write_one_signatured_type_data *data = (write_one_signatured_type_data *) d;
-    struct signatured_type_index_data *info = &data->info;
-    struct signatured_type *entry = (struct signatured_type *) *slot;
-
-    data->nametable.write_one_signatured_type (entry, info);
-
-    return 1;
-  }
-
 private:
 
   /* Storage for symbol names mapping them to their .debug_str section
@@ -1210,25 +1169,6 @@ class debug_names
       }
   }
 
-  /* A helper function that writes a single signatured_type
-     to a debug_names.  */
-  void
-  write_one_signatured_type (struct signatured_type *entry,
-			     struct signatured_type_index_data *info)
-  {
-    partial_symtab *psymtab = entry->v.psymtab;
-
-    write_psymbols (info->psyms_seen, psymtab->global_psymbols,
-		    info->cu_index, false, unit_kind::tu);
-    write_psymbols (info->psyms_seen, psymtab->static_psymbols,
-		    info->cu_index, true, unit_kind::tu);
-
-    info->types_list.append_uint (dwarf5_offset_size (), m_dwarf5_byte_order,
-				  to_underlying (entry->sect_off));
-
-    ++info->cu_index;
-  }
-
   /* Store value of each symbol.  */
   std::unordered_map<c_str_view, std::set<symbol_value>, c_str_view_hasher>
     m_name_to_value_set;
@@ -1475,17 +1415,16 @@ write_debug_names (dwarf2_per_objfile *per_objfile,
      work here.  Also, the debug_types entries do not appear in
      all_comp_units, but only in their own hash table.  */
   data_buf cu_list;
+  data_buf types_cu_list;
   debug_names nametable (per_objfile, dwarf5_is_dwarf64, dwarf5_byte_order);
   std::unordered_set<partial_symbol *>
     psyms_seen (psyms_seen_size (per_objfile));
   int counter = 0;
+  int types_counter = 0;
   for (int i = 0; i < per_objfile->per_bfd->all_comp_units.size (); ++i)
     {
       const dwarf2_per_cu_data *per_cu
 	= per_objfile->per_bfd->all_comp_units[i].get ();
-      if (per_cu->is_debug_types)
-	continue;
-
       partial_symtab *psymtab = per_cu->v.psymtab;
 
       /* CU of a shared file from 'dwz -m' may be unused by this main
@@ -1494,29 +1433,17 @@ write_debug_names (dwarf2_per_objfile *per_objfile,
       if (psymtab == NULL)
 	continue;
 
+      int &this_counter = per_cu->is_debug_types ? types_counter : counter;
+      data_buf &this_list = per_cu->is_debug_types ? types_cu_list : cu_list;
+
       if (psymtab->user == NULL)
 	nametable.recursively_write_psymbols (objfile, psymtab, psyms_seen,
-					      counter);
-
-      cu_list.append_uint (nametable.dwarf5_offset_size (), dwarf5_byte_order,
-			   to_underlying (per_cu->sect_off));
-      ++counter;
-    }
+					      this_counter);
 
-  /* Write out the .debug_type entries, if any.  */
-  data_buf types_cu_list;
-  if (per_objfile->per_bfd->signatured_types)
-    {
-      debug_names::write_one_signatured_type_data sig_data (nametable,
-			signatured_type_index_data (types_cu_list, psyms_seen));
-
-      sig_data.info.objfile = objfile;
-      /* It is used only for gdb_index.  */
-      sig_data.info.symtab = nullptr;
-      sig_data.info.cu_index = 0;
-      htab_traverse_noresize (per_objfile->per_bfd->signatured_types.get (),
-			      debug_names::write_one_signatured_type,
-			      &sig_data);
+      this_list.append_uint (nametable.dwarf5_offset_size (),
+			     dwarf5_byte_order,
+			     to_underlying (per_cu->sect_off));
+      ++this_counter;
     }
 
   nametable.build ();
-- 
2.26.3

Re: [PATCH 2/4] Minor cleanup to addrmap_index_data::previous_valid

[Lancelot SIX]

Hi

On Sat, May 29, 2021 at 07:54:41AM -0600, Tom Tromey wrote:
> This changes addrmap_index_data::previous_valid to a bool, and
> initializes it inline.
> 
> 2021-05-28  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/index-write.c (struct addrmap_index_data)
> 	<previous_valid>: Now bool.  Initialize.
> 	(add_address_entry_worker): Update.
> 	(write_address_map): Update.
> ---
>  gdb/ChangeLog            |  7 +++++++
>  gdb/dwarf2/index-write.c | 12 +++---------
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
> index ffc49e8ba82..656742f2e0a 100644
> --- a/gdb/dwarf2/index-write.c
> +++ b/gdb/dwarf2/index-write.c
> @@ -422,7 +422,7 @@ struct addrmap_index_data
>    /* Non-zero if the previous_* fields are valid.
>       We can't write an entry until we see the next entry (since it is only then
>       that we know the end of the entry).  */

The comment could also change from “Non-zero” to “true” to reflect the
type change, or even “Indicates if the previous_* fields are valid”.

Lancelot.

> -  int previous_valid;
> +  bool previous_valid = false;
>    /* Index of the CU in the table of all CUs in the index file.  */
>    unsigned int previous_cu_index;
>    /* Start address of the CU.  */
> @@ -459,10 +459,10 @@ add_address_entry_worker (void *datap, CORE_ADDR start_addr, void *obj)
>        const auto it = data->cu_index_htab.find (pst);
>        gdb_assert (it != data->cu_index_htab.cend ());
>        data->previous_cu_index = it->second;
> -      data->previous_valid = 1;
> +      data->previous_valid = true;
>      }
>    else
> -    data->previous_valid = 0;
> +    data->previous_valid = false;
>  
>    return 0;
>  }
> @@ -477,12 +477,6 @@ write_address_map (dwarf2_per_bfd *per_bfd, data_buf &addr_vec,
>  {
>    struct addrmap_index_data addrmap_index_data (addr_vec, cu_index_htab);
>  
> -  /* When writing the address table, we have to cope with the fact that
> -     the addrmap iterator only provides the start of a region; we have to
> -     wait until the next invocation to get the start of the next region.  */
> -
> -  addrmap_index_data.previous_valid = 0;
> -
>    addrmap_foreach (per_bfd->partial_symtabs->psymtabs_addrmap,
>  		   add_address_entry_worker, &addrmap_index_data);
>  
> -- 
> 2.26.3
> 

Re: [PATCH 2/4] Minor cleanup to addrmap_index_data::previous_valid

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <lsix@lancelotsix.com> writes:

Lancelot> The comment could also change from “Non-zero” to “true” to reflect the
Lancelot> type change, or even “Indicates if the previous_* fields are valid”.

Thanks, I changed it to use the "true" formulation.

Tom

Re: [PATCH 0/4] Some small debug index writer cleanups

[Tom Tromey]

Tom> I'm working on a patch series that removes the DWARF psymtab reader.
Tom> While updating the index writers, I found some small things that would
Tom> be convenient to fix for that work, and that could be submitted
Tom> separately.  This series is the result.

I'm checking these in now.

Tom