Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>
To: gdb-patches@sourceware.org
Cc: Tom Tromey <tom@tromey.com>
Subject: [PATCH, v2][gdb/symtab] Force usage of all_comp_units.size (CU/TU/CUTU)
Date: Thu, 21 Jul 2022 13:20:52 +0200	[thread overview]
Message-ID: <da8c7473-f2c5-2957-3e62-b6a088042cb8@suse.de> (raw)
In-Reply-To: <3b04b910-f020-7dba-54cd-46e23eb9657c@suse.de>

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

[ was: Re: [committed][gdb/symtab] Fix bad compile unit index complaint ]

On 7/21/22 13:10, Tom de Vries wrote:
> [ was: Re: [PATCH][gdb/symtab] Force usage of all_comp_units.size 
> (CU/TU/CUTU) ]
> 
> On 7/14/22 16:43, Tom de Vries wrote:
>> 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.
>>
> 
> I've dropped the all_comp_units.size (CU/TU/CUTU) part, since that 
> somewhat violates the One-Patch-Per-Independent=Change rule.

And here is it, independent of the dw2_debug_names_iterator::next issue.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-symtab-Force-usage-of-all_comp_units.size-CU-TU-CUTU.patch --]
[-- Type: text/x-patch, Size: 13233 bytes --]

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

It's 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)".

Tested on x86_64-linux.

---
 gdb/dwarf2/index-write.c | 11 +++---
 gdb/dwarf2/read.c        | 87 +++++++++++++++++++++++++++++-------------------
 gdb/dwarf2/read.h        | 32 +++++++++++++++++-
 3 files changed, 88 insertions(+), 42 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 42230607fe0..3dbf306ba86 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;
 }
@@ -4970,24 +4992,20 @@ dw2_debug_names_iterator::next ()
       switch (attr.dw_idx)
 	{
 	case DW_IDX_compile_unit:
-	  {
-	    /* Don't crash on bad data.  */
-	    int nr_cus = (per_bfd->all_comp_units.size ()
-			  - per_bfd->tu_stats.nr_tus);
-	    if (ull >= nr_cus)
-	      {
-		complaint (_(".debug_names entry has bad CU index %s"
-			     " [in module %s]"),
-			   pulongest (ull),
-			   objfile_name (objfile));
-		continue;
-	      }
-	  }
+	  /* Don't crash on bad data.  */
+	  if (ull >= per_bfd->all_comp_units.size (CU))
+	    {
+	      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;
 	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]"),
@@ -4996,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;
@@ -5322,7 +5339,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;
@@ -5617,7 +5634,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;
 
@@ -6873,7 +6890,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
@@ -6900,7 +6917,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)
     {
@@ -7056,7 +7073,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 ());
@@ -23434,12 +23451,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;
@@ -23483,7 +23500,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 ());
@@ -23518,7 +23535,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.  */

  reply	other threads:[~2022-07-21 11:21 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-14 14:43 [PATCH][gdb/symtab] " 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   ` Tom de Vries via Gdb-patches [this message]
2022-08-08 14:26 ` [PATCH][gdb/symtab] Force usage of all_comp_units.size (CU/TU/CUTU) Tom de Vries via Gdb-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=da8c7473-f2c5-2957-3e62-b6a088042cb8@suse.de \
    --to=gdb-patches@sourceware.org \
    --cc=tdevries@suse.de \
    --cc=tom@tromey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox