[PATCH] gdb: ensure has dwarf info before reading DWZ file

[PATCH] gdb: ensure has dwarf info before reading DWZ file

[Lancelot SIX]

I recent change (e9b738dfbdc "Avoid race when reading dwz file") moved
the call to dwarf2_read_dwz_file from dwarf2_initialize_objfile to
dwarf2_has_info.

Before that patch, dwarf2_initialize_objfile was only called when
dwarf2_has_info returned true, and since that patch it is always called.

When reading a file that has no debug info (.debug_info/.debug_abbrev
sections), but has a .gnu_debugaltlink section, GDB’s behavior is
different.  I can observe this when loading
/lib/x86_64-linux-gnu/libtinfo.so on Ubuntu 22.04 (or while debugging
any program dynamically loading this library).

Before e9b738dfbdc, we had:

    $ ./gdb/gdb -data-directory ./gdb/data-directory -q  /lib/x86_64-linux-gnu/libtinfo.so
    Reading symbols from /lib/x86_64-linux-gnu/libtinfo.so...
    (No debugging symbols found in /lib/x86_64-linux-gnu/libtinfo.so)
    (gdb)

while after we have:

    $ ./gdb/gdb -data-directory ./gdb/data-directory -q  /lib/x86_64-linux-gnu/libtinfo.so
    Reading symbols from /lib/x86_64-linux-gnu/libtinfo.so...

    warning: could not find '.gnu_debugaltlink' file for /usr/lib/x86_64-linux-gnu/libtinfo.so.6.3
    (No debugging symbols found in /lib/x86_64-linux-gnu/libtinfo.so)
    (gdb)

This patch restores the previous behavior of only trying to load the
DWZ file for objfiles when the main part of the debuginfo is present
(i.e. when dwarf2_has_info returns true).  We still make sure that
dwarf2_read_dwz_file is called at most once per objfile.

A consequence of this change is that the per_bfd->dwz_file optional
object can now remain empty (instead of containing a nullptr), so also
this patch also adjusts dwarf2_get_dwz_file to account for this
possibility.  This effectively reverts the changes to
dwarf2_get_dwz_file done by e9b738dfbdc.

Regression tested on x86_64-linux-gnu Ubuntu 22.04.
---
 gdb/dwarf2/dwz.c  | 13 +++++++++----
 gdb/dwarf2/read.c | 36 +++++++++++++++++++-----------------
 2 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/gdb/dwarf2/dwz.c b/gdb/dwarf2/dwz.c
index 1eb4816fb47..c7cdba24076 100644
--- a/gdb/dwarf2/dwz.c
+++ b/gdb/dwarf2/dwz.c
@@ -282,9 +282,14 @@ dwarf2_read_dwz_file (dwarf2_per_objfile *per_objfile)
 struct dwz_file *
 dwarf2_get_dwz_file (dwarf2_per_bfd *per_bfd, bool require)
 {
-  gdb_assert (per_bfd->dwz_file.has_value ());
-  dwz_file *result = per_bfd->dwz_file->get ();
-  if (require && result == nullptr)
-    error (_("could not read '.gnu_debugaltlink' section"));
+  gdb_assert (!require || per_bfd->dwz_file.has_value ());
+
+  dwz_file *result = nullptr;
+  if (per_bfd->dwz_file.has_value ())
+    {
+      result = per_bfd->dwz_file->get ();
+      if (require && result == nullptr)
+	error (_("could not read '.gnu_debugaltlink' section"));
+    }
   return result;
 }
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 31313bc88b3..9e37011ddf0 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1362,11 +1362,11 @@ dwarf2_has_info (struct objfile *objfile,
     return false;
 
   dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
+  bool just_created = false;
 
   if (per_objfile == NULL)
     {
       dwarf2_per_bfd *per_bfd;
-      bool just_created = false;
 
       /* We can share a "dwarf2_per_bfd" with other objfiles if the
 	 BFD doesn't require relocations.
@@ -1398,27 +1398,29 @@ dwarf2_has_info (struct objfile *objfile,
 	}
 
       per_objfile = dwarf2_objfile_data_key.emplace (objfile, objfile, per_bfd);
+    }
+
+  const bool has_info = (!per_objfile->per_bfd->info.is_virtual
+			 && per_objfile->per_bfd->info.s.section != nullptr
+			 && !per_objfile->per_bfd->abbrev.is_virtual
+			 && per_objfile->per_bfd->abbrev.s.section != nullptr);
 
-      if (just_created)
+  if (just_created && has_info)
+    {
+      /* Try to fetch any potential dwz file early, while still on
+	 the main thread.  Also, be sure to do it just once per
+	 BFD, to avoid races.  */
+      try
 	{
-	  /* Try to fetch any potential dwz file early, while still on
-	     the main thread.  Also, be sure to do it just once per
-	     BFD, to avoid races.  */
-	  try
-	    {
-	      dwarf2_read_dwz_file (per_objfile);
-	    }
-	  catch (const gdb_exception_error &err)
-	    {
-	      warning (_("%s"), err.what ());
-	    }
+	  dwarf2_read_dwz_file (per_objfile);
+	}
+      catch (const gdb_exception_error &err)
+	{
+	  warning (_("%s"), err.what ());
 	}
     }
 
-  return (!per_objfile->per_bfd->info.is_virtual
-	  && per_objfile->per_bfd->info.s.section != NULL
-	  && !per_objfile->per_bfd->abbrev.is_virtual
-	  && per_objfile->per_bfd->abbrev.s.section != NULL);
+  return has_info;
 }
 
 /* See declaration.  */

base-commit: 13ed3225004896142dcb0fbe24411b66f17dfc8e
-- 
2.34.1

Re: [PATCH] gdb: ensure has dwarf info before reading DWZ file

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <lancelot.six@amd.com> writes:

Lancelot> A consequence of this change is that the per_bfd->dwz_file optional
Lancelot> object can now remain empty (instead of containing a nullptr), so also
Lancelot> this patch also adjusts dwarf2_get_dwz_file to account for this
Lancelot> possibility.  This effectively reverts the changes to
Lancelot> dwarf2_get_dwz_file done by e9b738dfbdc.

I'm curious about the motivation for this change.
Can dwarf2_get_dwz_file be called when dwarf2_has_info returns false?

Tom

RE: [PATCH] gdb: ensure has dwarf info before reading DWZ file

[Six, Lancelot]

[AMD Official Use Only - General]

> Lancelot> A consequence of this change is that the per_bfd->dwz_file
> optional
> Lancelot> object can now remain empty (instead of containing a nullptr), so
> also
> Lancelot> this patch also adjusts dwarf2_get_dwz_file to account for this
> Lancelot> possibility.  This effectively reverts the changes to
> Lancelot> dwarf2_get_dwz_file done by e9b738dfbdc.
>
> I'm curious about the motivation for this change.
> Can dwarf2_get_dwz_file be called when dwarf2_has_info returns false?
>
> Tom

It can be called when doing "save gdb-index" (dwarf2/index-write.c: save_gdb_index_command).  This case is exercised by "gdb.dwarf2/gdb-index-nodebug.exp".

Maybe a better approach could be to modify save_gdb_index_command to check if there is any debug info before calling write_dwarf_index, but the current try/catch approach is the construct currently taking core of that.  Having dwarf2_get_dwz_file return nullptr in that case does made sense to me, but I can also go another direction.

Best,
Lancelot.

Re: [PATCH] gdb: ensure has dwarf info before reading DWZ file

[Tom Tromey]

> It can be called when doing "save gdb-index" (dwarf2/index-write.c:
> save_gdb_index_command).  This case is exercised by
> "gdb.dwarf2/gdb-index-nodebug.exp".

> Maybe a better approach could be to modify save_gdb_index_command to
> check if there is any debug info before calling write_dwarf_index, but
> the current try/catch approach is the construct currently taking core
> of that.  Having dwarf2_get_dwz_file return nullptr in that case does
> made sense to me, but I can also go another direction.

This seems fine, I don't think you need to change anything.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] gdb: ensure has dwarf info before reading DWZ file

[Lancelot SIX]

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

Thanks,

I have just pushed this patch.

Best,
Lancelot.