[PATCH] Avoid race when reading dwz file

[PATCH] Avoid race when reading dwz file

[Tom Tromey]

PR gdb/31260 points out a race introduced by the background reading
changes.  If a given objfile is re-opened when it is already being
read, dwarf2_initialize_objfile will call dwarf2_read_dwz_file again,
causing the 'dwz_file' to be reset.

This patch fixes the problem by arranging to open the dwz just once:
when the dwarf2_per_bfd object is created.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31260
---
 gdb/dwarf2/dwz.c  | 15 ++++++---------
 gdb/dwarf2/read.c | 29 ++++++++++++++++++-----------
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/gdb/dwarf2/dwz.c b/gdb/dwarf2/dwz.c
index 5c855f4c265..f53a5b64c4a 100644
--- a/gdb/dwarf2/dwz.c
+++ b/gdb/dwarf2/dwz.c
@@ -188,6 +188,8 @@ dwarf2_read_dwz_file (dwarf2_per_objfile *per_objfile)
      only be run in the main thread.  */
   gdb_assert (is_main_thread ());
 
+  /* This should only be called once.  */
+  gdb_assert (!per_bfd->dwz_file.has_value ());
   /* Set this early, so that on error it remains NULL.  */
   per_bfd->dwz_file.emplace (nullptr);
 
@@ -281,14 +283,9 @@ 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 (!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"));
-    }
+  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"));
   return result;
 }
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 925a5ea4a5b..d37d392c90b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1367,6 +1367,7 @@ dwarf2_has_info (struct objfile *objfile,
   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.
@@ -1386,6 +1387,7 @@ dwarf2_has_info (struct objfile *objfile,
 	      per_bfd = new dwarf2_per_bfd (objfile->obfd.get (), names,
 					    can_copy);
 	      dwarf2_per_bfd_bfd_data_key.set (objfile->obfd.get (), per_bfd);
+	      just_created = true;
 	    }
 	}
       else
@@ -1393,9 +1395,25 @@ dwarf2_has_info (struct objfile *objfile,
 	  /* No sharing possible, create one specifically for this objfile.  */
 	  per_bfd = new dwarf2_per_bfd (objfile->obfd.get (), names, can_copy);
 	  dwarf2_per_bfd_objfile_data_key.set (objfile, per_bfd);
+	  just_created = true;
 	}
 
       per_objfile = dwarf2_objfile_data_key.emplace (objfile, objfile, per_bfd);
+
+      if (just_created)
+	{
+	  /* 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 ());
+	    }
+	}
     }
 
   return (!per_objfile->per_bfd->info.is_virtual
@@ -3204,17 +3222,6 @@ dwarf2_initialize_objfile (struct objfile *objfile,
 
   dwarf_read_debug_printf ("called");
 
-  /* Try to fetch any potential dwz file early, while still on the
-     main thread.  */
-  try
-    {
-      dwarf2_read_dwz_file (per_objfile);
-    }
-  catch (const gdb_exception_error &err)
-    {
-      warning (_("%s"), err.what ());
-    }
-
   /* If we're about to read full symbols, don't bother with the
      indices.  In this case we also don't care if some other debug
      format is making psymtabs, because they are all about to be
-- 
2.43.0

Re: [PATCH] Avoid race when reading dwz file

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> PR gdb/31260 points out a race introduced by the background reading
Tom> changes.  If a given objfile is re-opened when it is already being
Tom> read, dwarf2_initialize_objfile will call dwarf2_read_dwz_file again,
Tom> causing the 'dwz_file' to be reset.

Tom> This patch fixes the problem by arranging to open the dwz just once:
Tom> when the dwarf2_per_bfd object is created.

Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31260

I'm checking this in.

Tom

Re: [PATCH] Avoid race when reading dwz file

[Lancelot SIX]

On Fri, Mar 08, 2024 at 07:09:46AM -0700, Tom Tromey wrote:
> >>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> PR gdb/31260 points out a race introduced by the background reading
> Tom> changes.  If a given objfile is re-opened when it is already being
> Tom> read, dwarf2_initialize_objfile will call dwarf2_read_dwz_file again,
> Tom> causing the 'dwz_file' to be reset.
> 
> Tom> This patch fixes the problem by arranging to open the dwz just once:
> Tom> when the dwarf2_per_bfd object is created.
> 
> Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31260
> 
> I'm checking this in.
> 
> Tom

Hi Tom,

It looks like this patch introduces a slight behavior change, and I am
not sure if this is intentional (or desired).

Before this patch we had:

    bool
    dwarf2_initialize_objfile (...)
    {
      if (!dwarf2_has_info (...))
        return false;
      ...
         dwarf2_read_dwz_file (...);
    }

This meas that dwarf2_read_dwz_file is only called if dwarf2_has_info
returns true.

Since this patch, the call to dwarf2_read_dwz_file has been moved inside
dwarf2_has_info.  As a consequence, dwarf2_read_dwz_file can be called
in cases where dwarf2_has_info returns false.

I can observe this on ubuntu-22.06 when loading
/lib/x86_64-linux-gnu/libtinfo.so.6 (provided by libtinfo6).  That file
has no .debug_info section, but has a .gnu_debugaltlink section.  The
old logic would skip dwarf2_read_dwz_file because of the absence of
.debug_info, but the new behavior tries (and might fail) to load the
debug information referenced in .gnu_debugaltlink:

    $ ./gdb/gdb -data-directory ./gdb/data-directory -q /lib/x86_64-linux-gnu/libtinfo.so.6
    Reading symbols from /lib/x86_64-linux-gnu/libtinfo.so.6...
    
    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.6)
    (gdb)

I am not too sure if it makes sense to try to load the debug info
referenced in .gnu_debugaltlink if the "main" debug info section is
missing.  It can be better than nothing as it can provide some types
information, but it is definitely not sufficient for a "full" debug
session.

Should the call to dwarf2_read_dwz_file be guarded only if "main" debug
info sections are present?  I can prepare a patch doing this.

[Arguably, .gnu_debugaltlink should also be removed when stripping the
debug information, but it is not.]

Best,
Lancelot.

Re: [PATCH] Avoid race when reading dwz file

[Tom Tromey]

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

Lancelot> I am not too sure if it makes sense to try to load the debug info
Lancelot> referenced in .gnu_debugaltlink if the "main" debug info section is
Lancelot> missing.  It can be better than nothing as it can provide some types
Lancelot> information, but it is definitely not sufficient for a "full" debug
Lancelot> session.

Lancelot> Should the call to dwarf2_read_dwz_file be guarded only if "main" debug
Lancelot> info sections are present?  I can prepare a patch doing this.

I think that would be fine.  I'm happy to do it if you'd rather.

thanks,
Tom

RE: [PATCH] Avoid race when reading dwz file

[Six, Lancelot]

[AMD Official Use Only - General]

> I think that would be fine.  I'm happy to do it if you'd rather.
>
> thanks,
> Tom

Hi,

I have submitted https://sourceware.org/pipermail/gdb-patches/2024-March/207682.html.  Let me know what you think.

Thanks,
Lancelot.