Avoid race when reading dwz file
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-arm |
fail
|
Testing failed
|
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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(-)
Comments
>>>>> "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
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.
>>>>> "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
[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.
@@ -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;
}
@@ -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