Re: [PATCH v2] gdb/dwarf: dump cooked index contents in cooked_index_functions::dump

[Simon Marchi <simon.marchi@polymtl.ca>]

> With this patch I'm seeing this failure:
> 
> 
> (gdb) PASS: gdb.dwarf2/dw2-error.exp: file dw2-error
> maint print objfiles /tmp/build/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error
> 
> Object file /tmp/build/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error:  Objfile at 0x40dada0, bfd at 0x426bf70, 50 minsyms
> 
> Cooked index in use:
> 
> ../../src/gdb/../gdbsupport/gdb-checked-static-cast.h:58: internal-error: checked_static_cast: Assertion `result != nullptr' failed.

Ah, sorry about that.

This is because dwarf2_build_psymtabs_hard throws, because of a bad
DWARF header (this is the point of the test), so no index ever gets
installed in index_table.

The obvious fix, to fix the test quickly, would be to add a nullptr
check in cooked_index_functions::dump.  However, the result is weird:

    (gdb) maintenance print objfiles

    Object file /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error:  Objfile at 0x614000007240, bfd at 0x6120000dbdc0, 24 minsyms

    Cooked index in use:

    (gdb)

It says the cooked index is in use, but there's nothing, as we never
actually managed to build a cooked index.  Maybe we should consider that
an invalid state, and remove the cooked_index_functions instance from
the objfile's quick symbol functions, if failing to read the psymtabs.

Of course, this state (cooked_index_functions in the objfile's qf +
per_objfile->index_table == nullptr) exists before
objfile::require_partial_symbols has been called on the objfile, for
quick symbol functions that can read partial symbols lazily (i.e. just
the DWARF cooked index at this time), as we haven't tried to read
partial symbols yet.  But after we called
objfile::require_partial_symbols, I don't think it's a state that makes
sense.

See the patch below for a quick prototype.
quick_symbol_functions::read_partial_symbols now returns a bool, and if
it returns false (failure), objfile::require_partial_symbols removes
that quick symbol instance from the qf list.

The result is:

    (gdb) file testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error
    Reading symbols from testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error...
    Dwarf Error: wrong version in compilation unit header (is 153, should be 2, 3, 4 or 5) [in module /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error]
    (No debugging symbols found in testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error)

The "No debugging symbols" message now appears.  It wouldn't appear,
because objfile_has_symbols would return true, which is kind of a lie.
When the cooked index qf is removed, it returns false.

And the problematic command that crashes the test now shows:

    (gdb) maintenance print objfiles

    Object file /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/dw2-error/dw2-error:  Objfile at 0x614000007240, bfd at 0x6120000dbdc0, 24 minsyms

    (gdb)

Which is the same that would be shown if the objfile didn't have DWARF
info at all in the first place.

Please let me know if you think this approach makes sense, and if I'll
finish and polish the patch.

Simon

From ef55dbecacc7ec5f5e1cea82fc731b21e078783e Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Mon, 30 Jan 2023 21:38:28 -0500
Subject: [PATCH] fix

Change-Id: Ic6b741324cca13239ac84c5dbef474534ad08870
---
 gdb/dwarf2/read.c   | 14 ++++++++++----
 gdb/quick-symbol.h  |  8 ++++++--
 gdb/symfile-debug.c | 17 +++++++++++++----
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index e8a3e359adaa..118cbd450668 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -5422,13 +5422,13 @@ dwarf2_initialize_objfile (struct objfile *objfile)
 
 /* Build a partial symbol table.  */
 
-static void
+static bool
 dwarf2_build_psymtabs (struct objfile *objfile)
 {
   dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
 
   if (per_objfile->per_bfd->index_table != nullptr)
-    return;
+    return true;
 
   try
     {
@@ -5436,10 +5436,14 @@ dwarf2_build_psymtabs (struct objfile *objfile)
 
       /* (maybe) store an index in the cache.  */
       global_index_cache.store (per_objfile);
+
+      return true;
     }
   catch (const gdb_exception_error &except)
     {
       exception_print (gdb_stderr, except);
+
+      return false;
     }
 }
 
@@ -18622,10 +18626,12 @@ struct cooked_index_functions : public dwarf2_base_index_functions
     return true;
   }
 
-  void read_partial_symbols (struct objfile *objfile) override
+  bool read_partial_symbols (struct objfile *objfile) override
   {
     if (dwarf2_has_info (objfile, nullptr))
-      dwarf2_build_psymtabs (objfile);
+      return dwarf2_build_psymtabs (objfile);
+
+    return true;
   }
 };
 
diff --git a/gdb/quick-symbol.h b/gdb/quick-symbol.h
index a7fea2ccb494..41a34ec6e200 100644
--- a/gdb/quick-symbol.h
+++ b/gdb/quick-symbol.h
@@ -221,9 +221,13 @@ struct quick_symbol_functions
   }
 
   /* Read the partial symbols for OBJFILE.  This will only ever be
-     called if can_lazily_read_symbols returns true.  */
-  virtual void read_partial_symbols (struct objfile *objfile)
+     called if can_lazily_read_symbols returns true.
+
+     Return true on success.  Return false on failure, in which case the
+     quick_symbol_functions instance is removed from the objfile's QF list.  */
+  virtual bool read_partial_symbols (struct objfile *objfile)
   {
+    return true;
   }
 };
 
diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
index efc6bcdf2bdf..f244ec0d5d6e 100644
--- a/gdb/symfile-debug.c
+++ b/gdb/symfile-debug.c
@@ -313,7 +313,7 @@ objfile::dump ()
     gdb_printf (gdb_stdlog, "qf->dump (%s)\n",
 		objfile_debug_name (this));
 
-  for (const auto &iter : qf)
+  for (const auto &iter : this->qf_require_partial_symbols ())
     iter->dump (this);
 }
 
@@ -528,9 +528,10 @@ objfile::require_partial_symbols (bool verbose)
       flags |= OBJF_PSYMTABS_READ;
 
       bool printed = false;
-      for (const auto &iter : qf)
+      for (auto iter = qf.begin (), prev = qf.before_begin ();
+	   iter != qf.end (); )
 	{
-	  if (iter->can_lazily_read_symbols ())
+	  if ((*iter)->can_lazily_read_symbols ())
 	    {
 	      if (verbose && !printed)
 		{
@@ -538,7 +539,15 @@ objfile::require_partial_symbols (bool verbose)
 			      objfile_name (this));
 		  printed = true;
 		}
-	      iter->read_partial_symbols (this);
+
+	      bool success = (*iter)->read_partial_symbols (this);
+	      if (success)
+		{
+		  ++iter;
+		  ++prev;
+		}
+	      else
+		iter = qf.erase_after (prev);
 	    }
 	}
       if (printed && !objfile_has_symbols (this))

base-commit: 9c6e6c8f4b07a51a47c10a84e10a938b64b65fd5
-- 
2.39.0