Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH][gdb/symtab] Force usage of all_comp_units.size (CU/TU/CUTU)
@ 2022-07-14 14:43 Tom de Vries via Gdb-patches
  2022-07-21 11:10 ` [committed][gdb/symtab] Fix bad compile unit index complaint Tom de Vries via Gdb-patches
  2022-08-08 14:26 ` [PATCH][gdb/symtab] " Tom de Vries via Gdb-patches
  0 siblings, 2 replies; 4+ messages in thread
From: Tom de Vries via Gdb-patches @ 2022-07-14 14:43 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

Hi,

I noticed this code in dw2_debug_names_iterator::next:
...
        case DW_IDX_compile_unit:
          /* Don't crash on bad data.  */
          if (ull >= per_bfd->all_comp_units.size ())
            {
              complaint (_(".debug_names entry has bad CU index %s"
                           " [in module %s]"),
                         pulongest (ull),
                         objfile_name (objfile));
              continue;
            }
          per_cu = per_bfd->get_cu (ull);
          break;
...

This code used to DTRT, before we started keeping both CUs and TUs in
all_comp_units.

It's just too easy to read "all_comp_units.size ()" and only think about CUs
and forget about TUs.

Introduce "all_comp_units.size (CU/TU/CUTU)", to make it explicit which of the
three we mean at every use, and replace:
- uses of tu_stats.nr_tus with all_comp_units.size (TU)
  (apart from the use in print_tu_stats),
- uses of (all_comp_units.size () - tu_stats.nr_tus)
  with all_comp_units.size (CU), and
- all other uses of "all_comp_units.size ()" with "all_comp_units.size (CUTU)".

Fix the problem in dw2_debug_names_iterator::next by using
"all_comp_units.size (CU)".

It's hard to produce a test-case for this, but let's try at least to trigger
the complaint somehow.  We start out by creating an exec with .debug_types and
.debug_names:
...
$ gcc -g ~/hello.c -fdebug-types-section
$ gdb-add-index -dwarf-5 a.out
...
and verify that we don't see any complaints:
...
$ gdb -q -batch -iex "set complaints 100" ./a.out
...

We look at the CU and TU table using readelf -w and conclude that we have
nr_cus == 6 and nr_tus == 1.

Now override ull in dw2_debug_names_iterator::next for the DW_IDX_compile_unit
case to 6, and we have:
...
$ gdb -q -batch -iex "set complaints 100" ./a.out
During symbol reading: .debug_names entry has bad CU index 6 [in module a.out]
...

[ After this, it still crashes because this code in
dw2_debug_names_iterator::next:
...
  /* Skip if already read in.  */
  if (m_per_objfile->symtab_set_p (per_cu))
    goto again;
...
is called with per_cu == nullptr, but I consider this a separate issue. ]

Now revert the fix (all_comp_units.size (CU) -> all_comp_units.size (CUTU))
and observe that the complaint disappears, so we've confirmed that the fix is
required.

There's possibly a similar issue for .gdb_index in dw2_symtab_iter_next, filed
as PR29367.

Tested on x86_64-linux, with native and target board cc-with-debug-names.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29336

Any comments?

Thanks,
- Tom

[gdb/symtab] Force usage of all_comp_units.size (CU/TU/CUTU)

---
 gdb/dwarf2/index-write.c | 11 ++++----
 gdb/dwarf2/read.c        | 67 +++++++++++++++++++++++++++++++-----------------
 gdb/dwarf2/read.h        | 32 ++++++++++++++++++++++-
 3 files changed, 80 insertions(+), 30 deletions(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index efd154d41df..fceef3d0b1e 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1176,7 +1176,7 @@ write_gdbindex (dwarf2_per_objfile *per_objfile,
      in the index file).  This will later be needed to write the address
      table.  */
   cu_index_map cu_index_htab;
-  cu_index_htab.reserve (per_objfile->per_bfd->all_comp_units.size ());
+  cu_index_htab.reserve (per_objfile->per_bfd->all_comp_units.size (CUTU));
 
   /* Store out the .debug_type CUs, if any.  */
   data_buf types_cu_list;
@@ -1187,7 +1187,7 @@ write_gdbindex (dwarf2_per_objfile *per_objfile,
 
   int counter = 0;
   int types_counter = 0;
-  for (int i = 0; i < per_objfile->per_bfd->all_comp_units.size (); ++i)
+  for (int i = 0; i < per_objfile->per_bfd->all_comp_units.size (CUTU); ++i)
     {
       dwarf2_per_cu_data *per_cu
 	= per_objfile->per_bfd->all_comp_units[i].get ();
@@ -1270,7 +1270,7 @@ write_debug_names (dwarf2_per_objfile *per_objfile,
   debug_names nametable (per_objfile, dwarf5_is_dwarf64, dwarf5_byte_order);
   int counter = 0;
   int types_counter = 0;
-  for (int i = 0; i < per_objfile->per_bfd->all_comp_units.size (); ++i)
+  for (int i = 0; i < per_objfile->per_bfd->all_comp_units.size (CUTU); ++i)
     {
       dwarf2_per_cu_data *per_cu
 	= per_objfile->per_bfd->all_comp_units[i].get ();
@@ -1286,9 +1286,8 @@ write_debug_names (dwarf2_per_objfile *per_objfile,
     }
 
    /* Verify that all units are represented.  */
-  gdb_assert (counter == (per_objfile->per_bfd->all_comp_units.size ()
-			  - per_objfile->per_bfd->tu_stats.nr_tus));
-  gdb_assert (types_counter == per_objfile->per_bfd->tu_stats.nr_tus);
+  gdb_assert (counter == per_objfile->per_bfd->all_comp_units.size (CU));
+  gdb_assert (types_counter == per_objfile->per_bfd->all_comp_units.size (TU));
 
   for (const cooked_index_entry *entry : table->all_entries ())
     nametable.insert (entry);
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 5ca787cc095..e554bc4f642 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1391,7 +1391,7 @@ class all_comp_units_iterator
 
   all_comp_units_iterator (dwarf2_per_bfd *per_bfd, bool start)
     : m_per_bfd (per_bfd),
-      m_index (start ? 0 : per_bfd->all_comp_units.size ())
+      m_index (start ? 0 : per_bfd->all_comp_units.size (CUTU))
   {
   }
 
@@ -1448,11 +1448,33 @@ class all_comp_units_range
   dwarf2_per_bfd *m_per_bfd;
 };
 
+/* See read.h.  */
+
+all_comp_units_type::size_type
+all_comp_units_type::size (all_comp_units_size_type what) const noexcept
+{
+  all_comp_units_type::size_type nr_cus_tus
+    = ((std::vector<dwarf2_per_cu_data_up> *)(this))->size ();
+
+  if (what == CUTU)
+    return nr_cus_tus;
+
+  gdb_assert (m_per_bfd != nullptr);
+  size_type nr_tus = m_per_bfd->tu_stats.nr_tus;
+  if (what == CU)
+    return nr_cus_tus - nr_tus;
+  else if (what == TU)
+    return nr_tus;
+  else
+    gdb_assert_not_reached ("");
+}
+
 /* See declaration.  */
 
 dwarf2_per_bfd::dwarf2_per_bfd (bfd *obfd, const dwarf2_debug_sections *names,
 				bool can_copy_)
   : obfd (obfd),
+    all_comp_units (this),
     can_copy (can_copy_)
 {
   if (names == NULL)
@@ -2092,7 +2114,7 @@ dwarf2_per_bfd::allocate_per_cu ()
 {
   dwarf2_per_cu_data_up result (new dwarf2_per_cu_data);
   result->per_bfd = this;
-  result->index = all_comp_units.size ();
+  result->index = all_comp_units.size (CUTU);
   return result;
 }
 
@@ -2103,7 +2125,7 @@ dwarf2_per_bfd::allocate_signatured_type (ULONGEST signature)
 {
   signatured_type_up result (new signatured_type (signature));
   result->per_bfd = this;
-  result->index = all_comp_units.size ();
+  result->index = all_comp_units.size (CUTU);
   result->is_debug_types = true;
   tu_stats.nr_tus++;
   return result;
@@ -2297,7 +2319,7 @@ create_addrmap_from_index (dwarf2_per_objfile *per_objfile,
 	  continue;
 	}
 
-      if (cu_index >= per_bfd->all_comp_units.size ())
+      if (cu_index >= per_bfd->all_comp_units.size (CUTU))
 	{
 	  complaint (_(".gdb_index address table has invalid CU number %u"),
 		     (unsigned) cu_index);
@@ -2708,7 +2730,7 @@ dwarf2_read_gdb_index
 
   per_bfd->index_table = std::move (map);
   per_bfd->quick_file_names_table =
-    create_quick_file_names_table (per_bfd->all_comp_units.size ());
+    create_quick_file_names_table (per_bfd->all_comp_units.size (CUTU));
 
   return 1;
 }
@@ -2975,7 +2997,7 @@ dw2_symtab_iter_next (struct dw2_symtab_iterator *iter,
 			 && symbol_kind != GDB_INDEX_SYMBOL_KIND_NONE);
 
       /* Don't crash on bad data.  */
-      if (cu_index >= per_objfile->per_bfd->all_comp_units.size ())
+      if (cu_index >= per_objfile->per_bfd->all_comp_units.size (CUTU))
 	{
 	  complaint (_(".gdb_index entry has bad CU index"
 		       " [in module %s]"), objfile_name (per_objfile->objfile));
@@ -3056,7 +3078,7 @@ dwarf2_base_index_functions::print_stats (struct objfile *objfile,
     return;
 
   dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
-  int total = per_objfile->per_bfd->all_comp_units.size ();
+  int total = per_objfile->per_bfd->all_comp_units.size (CUTU);
   int count = 0;
 
   for (int i = 0; i < total; ++i)
@@ -3090,7 +3112,7 @@ void
 dwarf2_base_index_functions::expand_all_symtabs (struct objfile *objfile)
 {
   dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
-  int total_units = per_objfile->per_bfd->all_comp_units.size ();
+  int total_units = per_objfile->per_bfd->all_comp_units.size (CUTU);
 
   for (int i = 0; i < total_units; ++i)
     {
@@ -4053,7 +4075,7 @@ dw2_expand_marked_cus
 	}
 
       /* Don't crash on bad data.  */
-      if (cu_index >= per_objfile->per_bfd->all_comp_units.size ())
+      if (cu_index >= per_objfile->per_bfd->all_comp_units.size (CUTU))
 	{
 	  complaint (_(".gdb_index entry has bad CU index"
 		       " [in module %s]"), objfile_name (per_objfile->objfile));
@@ -4713,7 +4735,7 @@ dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
 
   per_bfd->index_table = std::move (map);
   per_bfd->quick_file_names_table =
-    create_quick_file_names_table (per_bfd->all_comp_units.size ());
+    create_quick_file_names_table (per_bfd->all_comp_units.size (CUTU));
 
   return true;
 }
@@ -4971,7 +4993,7 @@ dw2_debug_names_iterator::next ()
 	{
 	case DW_IDX_compile_unit:
 	  /* Don't crash on bad data.  */
-	  if (ull >= per_bfd->all_comp_units.size ())
+	  if (ull >= per_bfd->all_comp_units.size (CU))
 	    {
 	      complaint (_(".debug_names entry has bad CU index %s"
 			   " [in module %s]"),
@@ -4983,7 +5005,7 @@ dw2_debug_names_iterator::next ()
 	  break;
 	case DW_IDX_type_unit:
 	  /* Don't crash on bad data.  */
-	  if (ull >= per_bfd->tu_stats.nr_tus)
+	  if (ull >= per_bfd->all_comp_units.size (TU))
 	    {
 	      complaint (_(".debug_names entry has bad TU index %s"
 			   " [in module %s]"),
@@ -4992,8 +5014,7 @@ dw2_debug_names_iterator::next ()
 	      continue;
 	    }
 	  {
-	    int nr_cus = (per_bfd->all_comp_units.size ()
-			  - per_bfd->tu_stats.nr_tus);
+	    int nr_cus = per_bfd->all_comp_units.size (CU);
 	    per_cu = per_bfd->get_cu (nr_cus + ull);
 	  }
 	  break;
@@ -5314,7 +5335,7 @@ dwarf2_initialize_objfile (struct objfile *objfile)
 
       create_all_comp_units (per_objfile);
       per_bfd->quick_file_names_table
-	= create_quick_file_names_table (per_bfd->all_comp_units.size ());
+	= create_quick_file_names_table (per_bfd->all_comp_units.size (CUTU));
 
       objfile->qf.emplace_front (new readnow_functions);
       return;
@@ -5609,7 +5630,7 @@ create_debug_types_hash_table (dwarf2_per_objfile *per_objfile,
 static struct signatured_type *
 add_type_unit (dwarf2_per_objfile *per_objfile, ULONGEST sig, void **slot)
 {
-  if (per_objfile->per_bfd->all_comp_units.size ()
+  if (per_objfile->per_bfd->all_comp_units.size (CUTU)
       == per_objfile->per_bfd->all_comp_units.capacity ())
     ++per_objfile->per_bfd->tu_stats.nr_all_type_units_reallocs;
 
@@ -6865,7 +6886,7 @@ build_type_psymtabs (dwarf2_per_objfile *per_objfile,
   /* It's up to the caller to not call us multiple times.  */
   gdb_assert (per_objfile->per_bfd->type_unit_groups == NULL);
 
-  if (per_objfile->per_bfd->tu_stats.nr_tus == 0)
+  if (per_objfile->per_bfd->all_comp_units.size (TU) == 0)
     return;
 
   /* TUs typically share abbrev tables, and there can be way more TUs than
@@ -6892,7 +6913,7 @@ build_type_psymtabs (dwarf2_per_objfile *per_objfile,
   /* Sort in a separate table to maintain the order of all_comp_units
      for .gdb_index: TU indices directly index all_type_units.  */
   std::vector<tu_abbrev_offset> sorted_by_abbrev;
-  sorted_by_abbrev.reserve (per_objfile->per_bfd->tu_stats.nr_tus);
+  sorted_by_abbrev.reserve (per_objfile->per_bfd->all_comp_units.size (TU));
 
   for (const auto &cu : per_objfile->per_bfd->all_comp_units)
     {
@@ -7048,7 +7069,7 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
   std::vector<std::unique_ptr<cooked_index>> indexes;
 
   per_bfd->quick_file_names_table
-    = create_quick_file_names_table (per_bfd->all_comp_units.size ());
+    = create_quick_file_names_table (per_bfd->all_comp_units.size (CUTU));
   if (!per_bfd->debug_aranges.empty ())
     read_addrmap_from_aranges (per_objfile, &per_bfd->debug_aranges,
 			       index_storage.get_addrmap ());
@@ -23424,12 +23445,12 @@ static int
 dwarf2_find_containing_comp_unit
   (sect_offset sect_off,
    unsigned int offset_in_dwz,
-   const std::vector<dwarf2_per_cu_data_up> &all_comp_units)
+   const all_comp_units_type &all_comp_units)
 {
   int low, high;
 
   low = 0;
-  high = all_comp_units.size () - 1;
+  high = all_comp_units.size (CUTU) - 1;
   while (high > low)
     {
       struct dwarf2_per_cu_data *mid_cu;
@@ -23473,7 +23494,7 @@ dwarf2_find_containing_comp_unit (sect_offset sect_off,
     }
   else
     {
-      if (low == per_bfd->all_comp_units.size () - 1
+      if (low == per_bfd->all_comp_units.size (CUTU) - 1
 	  && sect_off >= this_cu->sect_off + this_cu->length ())
 	error (_("invalid dwarf2 offset %s"), sect_offset_str (sect_off));
       gdb_assert (sect_off < this_cu->sect_off + this_cu->length ());
@@ -23508,7 +23529,7 @@ run_test ()
   four->set_length (7);
   four->is_dwz = 1;
 
-  std::vector<dwarf2_per_cu_data_up> units;
+  all_comp_units_type units (nullptr);
   units.push_back (std::move (one));
   units.push_back (std::move (two));
   units.push_back (std::move (three));
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index c2f86a9d367..89e8ad29758 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -410,6 +410,36 @@ struct signatured_type : public dwarf2_per_cu_data
 
 using signatured_type_up = std::unique_ptr<signatured_type>;
 
+/* To specify whether we want the number of CUs, TUs, or both CUs and TUs from
+   all_comp_units.size ().  */
+
+enum all_comp_units_size_type
+{
+  CU,
+  TU,
+  CUTU
+};
+
+/* Variant of std::vector<dwarf2_per_cu_data_up> that deletes the size ()
+   member function and replaces it with size (all_comp_units_size_type).
+   This forces us to specify explicitly whether we want the number of CUs, TUs
+   or the total at every use.  */
+
+class all_comp_units_type : public std::vector<dwarf2_per_cu_data_up>
+{
+public:
+  all_comp_units_type (struct dwarf2_per_bfd *per_bfd)
+  {
+    m_per_bfd = per_bfd;
+  }
+
+  size_type size () const noexcept = delete;
+  size_type size (all_comp_units_size_type what) const noexcept;
+
+private:
+  struct dwarf2_per_bfd *m_per_bfd = nullptr;
+};
+
 /* Some DWARF data can be shared across objfiles who share the same BFD,
    this data is stored in this object.
 
@@ -489,7 +519,7 @@ struct dwarf2_per_bfd
 
   /* Table of all the compilation units.  This is used to locate
      the target compilation unit of a particular reference.  */
-  std::vector<dwarf2_per_cu_data_up> all_comp_units;
+  all_comp_units_type all_comp_units;
 
   /* Table of struct type_unit_group objects.
      The hash key is the DW_AT_stmt_list value.  */

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-08 14:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-14 14:43 [PATCH][gdb/symtab] Force usage of all_comp_units.size (CU/TU/CUTU) Tom de Vries via Gdb-patches
2022-07-21 11:10 ` [committed][gdb/symtab] Fix bad compile unit index complaint Tom de Vries via Gdb-patches
2022-07-21 11:20   ` [PATCH, v2][gdb/symtab] Force usage of all_comp_units.size (CU/TU/CUTU) Tom de Vries via Gdb-patches
2022-08-08 14:26 ` [PATCH][gdb/symtab] " Tom de Vries via Gdb-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox