gdb/dwarf: clear per_bfd::num_{comp,type}_units on error

Commit bedd6a7a44 ("gdb/dwarf: track compilation and type unit count")
causes this internal error:

    $ ./gdb -nx -q --data-directory=data-directory testsuite/outputs/gdb.dwarf2/debug-names-duplicate-cu/debug-names-duplicate-cu -ex "save gdb-index -dwarf-5 /tmp" -batch

    warning: Section .debug_names has incorrect number of CUs in CU table, ignoring .debug_names.
    /home/smarchi/src/binutils-gdb/gdb/dwarf2/index-write.c:1454: internal-error: write_debug_names: Assertion `comp_unit_counter == per_bfd->num_comp_units' failed.

This is visible when running this test:

    $ make check TESTS="gdb.dwarf2/debug-names-duplicate-cu.exp" RUNTESTFLAGS="--target_board=cc-with-debug-names"
    ...
    Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/debug-names-duplicate-cu.exp ...
    gdb compile failed, warning: Section .debug_names has incorrect number of CUs in CU table, ignoring .debug_names.
    /home/smarchi/src/binutils-gdb/gdb/dwarf2/index-write.c:1454: internal-error: write_debug_names: Assertion `comp_unit_counter == per_bfd->num_comp_units' failed.
    ...
                    === gdb Summary ===

    # of untested testcases         1

However, it's easy to miss because it only causes an "UNTESTED" to be
recorded, not a FAIL or UNRESOLVED.  This is because the problem happens
while trying to create the .debug_names index, as part of the test case
compilation.

The problem is: when we bail out from using .debug_names because we
detect it is inconsistent with the units in .debug_info, we clear
per_bfd->all_units, to destroy all units previously created, before
proceeding to read the units with an index.  However, we don't clear
per_bfd->num_{comp,type}_units.  As a result, per_bfd->all_units
contains one unit, while per_bfd->num_comp_units is 2.  Whenever we
clear per_bfd->all_units, we should also clear
per_bfd->num_{comp,type}_units.

While at it, move this logic inside a scoped object.

I added an assertion in finalize_all_units to verify that the size of
per_bfd->all_units equals per_bfd->num_comp_units +
per_bfd->num_type_units.  This makes the problem (if omitting the fix)
visible when running gdb.dwarf2/debug-names-duplicate-cu.exp with the
unix (default) target board:

    ERROR: Couldn't load debug-names-duplicate-cu into GDB (GDB internal error).
    FAIL: gdb.dwarf2/debug-names-duplicate-cu.exp: find index type (GDB internal error)
    FAIL: gdb.dwarf2/debug-names-duplicate-cu.exp: find index type, check type is valid

                    === gdb Summary ===

    # of expected passes            1
    # of unexpected failures        2
    # of unresolved testcases       1

I considered changing the code to build a local vector of units first,
then move it in per_bfd->all_units on success, that would avoid having
to clean it up on error.  I did not do it because it's a much larger
change, but we could consider it.

Change-Id: I49bcc0cb4b34aba3d882b27c8a93c168e8875c08
Approved-By: Tom Tromey <tom@tromey.com>

4 files changed, 44 insertions, 21 deletions

diff --git a/gdb/dwarf2/read-debug-names.c b/gdb/dwarf2/read-debug-names.c
index 97677c0..ddf4935 100644
--- a/gdb/dwarf2/read-debug-names.c
+++ b/gdb/dwarf2/read-debug-names.c
@@ -768,12 +768,12 @@ build_and_check_cu_lists_from_debug_names (dwarf2_per_bfd *per_bfd,
   return build_and_check_cu_list_from_debug_names (per_bfd, dwz_map, dwz->info);
 }
 
-/* This does all the work for dwarf2_read_debug_names, but putting it
-   into a separate function makes some cleanup a bit simpler.  */
+/* See read-debug-names.h.  */
 
-static bool
-do_dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
+bool
+dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
 {
+  scoped_remove_all_units remove_all_units (*per_objfile->per_bfd);
   mapped_debug_names_reader map;
   mapped_debug_names_reader dwz_map;
   struct objfile *objfile = per_objfile->objfile;
@@ -850,17 +850,7 @@ do_dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
 	       (per_objfile, std::move (map)));
   auto idx = std::make_unique<debug_names_index> (std::move (cidn));
   per_bfd->start_reading (std::move (idx));
+  remove_all_units.disable ();
 
   return true;
 }
-
-/* See read-debug-names.h.  */
-
-bool
-dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
-{
-  bool result = do_dwarf2_read_debug_names (per_objfile);
-  if (!result)
-    per_objfile->per_bfd->all_units.clear ();
-  return result;
-}
diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
index 7dfba73..fc7b654 100644
--- a/gdb/dwarf2/read-gdb-index.c
+++ b/gdb/dwarf2/read-gdb-index.c
@@ -1489,6 +1489,7 @@ dwarf2_read_gdb_index
   offset_type cu_list_elements, types_list_elements, dwz_list_elements = 0;
   struct objfile *objfile = per_objfile->objfile;
   dwarf2_per_bfd *per_bfd = per_objfile->per_bfd;
+  scoped_remove_all_units remove_all_units (*per_bfd);
 
   gdb::array_view<const gdb_byte> main_index_contents
     = get_gdb_index_contents (objfile, per_bfd);
@@ -1544,10 +1545,7 @@ dwarf2_read_gdb_index
 	 an index.  */
       if (per_bfd->infos.size () > 1
 	  || per_bfd->types.size () > 1)
-	{
-	  per_bfd->all_units.clear ();
-	  return false;
-	}
+	return false;
 
       dwarf2_section_info *section
 	= (per_bfd->types.size () == 1
@@ -1566,6 +1564,7 @@ dwarf2_read_gdb_index
   set_main_name_from_gdb_index (per_objfile, map.get ());
 
   per_bfd->index_table = std::move (map);
+  remove_all_units.disable ();
 
   return true;
 }
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index c37da59..7bd0850 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -3679,6 +3679,10 @@ read_comp_units_from_section (dwarf2_per_objfile *per_objfile,
 void
 finalize_all_units (dwarf2_per_bfd *per_bfd)
 {
+  /* Sanity check.  */
+  gdb_assert (per_bfd->all_units.size ()
+	      == per_bfd->num_comp_units + per_bfd->num_type_units);
+
   /* Ensure that the all_units vector is in the expected order for
      dwarf2_find_containing_unit to be able to perform a binary search.  */
   std::sort (per_bfd->all_units.begin (), per_bfd->all_units.end (),
@@ -3694,6 +3698,7 @@ void
 create_all_units (dwarf2_per_objfile *per_objfile)
 {
   gdb_assert (per_objfile->per_bfd->all_units.empty ());
+  scoped_remove_all_units remove_all_units (*per_objfile->per_bfd);
 
   signatured_type_set sig_types;
 
@@ -3714,8 +3719,6 @@ create_all_units (dwarf2_per_objfile *per_objfile)
 
       if (!dwz->types.empty ())
 	{
-	  per_objfile->per_bfd->all_units.clear ();
-
 	  /* See enhancement PR symtab/30838.  */
 	  error (_(DWARF_ERROR_PREFIX
 		   ".debug_types section not supported in dwz file"));
@@ -3725,6 +3728,7 @@ create_all_units (dwarf2_per_objfile *per_objfile)
   per_objfile->per_bfd->signatured_types = std::move (sig_types);
 
   finalize_all_units (per_objfile->per_bfd);
+  remove_all_units.disable ();
 }
 
 /* Return the initial uleb128 in the die at INFO_PTR.  */
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 4e3f8d7..74ec420 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -673,6 +673,36 @@ public:
   std::string captured_debug_dir;
 };
 
+/* Scoped object to remove all units from PER_BFD and clear other associated
+   fields in case of failure.  */
+
+struct scoped_remove_all_units
+{
+  explicit scoped_remove_all_units (dwarf2_per_bfd &per_bfd)
+    : m_per_bfd (&per_bfd)
+  {}
+
+  DISABLE_COPY_AND_ASSIGN (scoped_remove_all_units);
+
+  ~scoped_remove_all_units ()
+  {
+    if (m_per_bfd == nullptr)
+      return;
+
+    m_per_bfd->all_units.clear ();
+    m_per_bfd->num_comp_units = 0;
+    m_per_bfd->num_type_units = 0;
+  }
+
+  /* Disable this object.  Call this to keep the units of M_PER_BFD on the
+     success path.  */
+  void disable () { m_per_bfd = nullptr; }
+
+private:
+  /* This is nullptr if the object is disabled.  */
+  dwarf2_per_bfd *m_per_bfd;
+};
+
 /* An iterator for all_units that is based on index.  This
    approach makes it possible to iterate over all_units safely,
    when some caller in the loop may add new units.  */