[PATCH v2 0/2] [gdb] TSAN fixes

[PATCH v2 0/2] [gdb] TSAN fixes

[Tom de Vries]

I build gdb with TSAN at -O0, ran the testsuite and found a few problems.

This series fixes those.

The first patch changes the locking scheme.

The second patch removes a local lock in try_open_dwop_file.

Changes since v1:
- commited an approved patch [1]
- combined two patches related to the local lock in try_open_dwop_file
- replaced last patch by new patch changing the locking scheme

[1] https://sourceware.org/pipermail/gdb-patches/2026-January/224422.html

Tom de Vries (2):
  [gdb/symtab] Replace per-BFD lock with global BFD lock
  [gdb/symtab] Fix data race in try_open_dwop_file

 gdb/dwarf2/read.c | 15 ++++-----------
 gdb/gdb_bfd.c     | 37 ++++++++++++++++---------------------
 gdb/gdb_bfd.h     |  4 ++++
 3 files changed, 24 insertions(+), 32 deletions(-)


base-commit: 2d306bf8a70e4ef9d6cd6eeecf17a8e6f4f04151
-- 
2.51.0

[PATCH v2 1/2] [gdb/symtab] Replace per-BFD lock with global BFD lock

[Tom de Vries]

Our current BFD locking scheme is as follows [1]:
...
There is one global mutex, gdb_bfd_mutex, which BFD can lock and unlock via
the callbacks we pass it.  This appears to lock the internal global data
structures of BFD (like its global cache or some global counter), but not data
in individual `bfd *`instances.  If the user of BFD wishes to call functions
on a given `bfd *` from multiple threads, it must provide the synchronization
itself.  For this, we have gdb_bfd_data::per_bfd_mutex.
...

PR33811 reports the following data race:
...
  Read of size 1 at 0x72440010c608 by thread T5 (mutexes: write M0):
    #0 bfd_get_section_limit_octets bfd.h:2433
    #1 bfd_get_section_contents bfd/section.c:1612
    #2 bfd_is_section_compressed_info bfd/compress.c:901
    #3 bfd_is_section_compressed bfd/compress.c:959
    #4 gdb_bfd_map_section(bfd_section*, unsigned long*) gdb/gdb_bfd.c:779
...
vs:
...
  Previous write of size 4 at 0x72440010c608 by main thread (mutexes: write M1):
    #0 bfd_cache_delete bfd/cache.c:180
    #1 _bfd_cache_close_unlocked bfd/cache.c:607
    #2 bfd_cache_close_all bfd/cache.c:664
    #3 notify_before_prompt gdb/event-top.c:524
...

In more detail, this read in bfd_get_section_limit_octets in bfd/bfd-in2.h:
...
  if (abfd->direction != write_direction && sec->rawsize != 0)
...
vs. this write in bfd_cache_delete in bfd/cache.c:
...
  abfd->last_io = bfd_io_force;
...

There is already locking used for both the read and write.

In gdb_bfd_map_section, we use the per-BFD lock:
...
  gdb_bfd_data *gdata = (gdb_bfd_data *) bfd_usrdata (abfd);
  gdb::lock_guard<gdb::mutex> guard (gdata->per_bfd_mutex);
...

And in bfd_cache_close_all, we use the global BFD lock:
...
bool
bfd_cache_close_all (void)
{
  ...
  if (!bfd_lock ())
    return false;
  ...
  if (!bfd_unlock ())
    return false;
  return ret;
}
...

The problem is that the locking is not sufficient.  Since bfd_cache_close_all
accesses individual BFDs, it needs to lock the corresponding per-BFD locks as
well.

A naive way to implement this using the existing scheme of wrappers, would be to
add a gdb_bfd_cache_close_all that locks all per-BFD locks, calls
bfd_cache_close_all, and unlocks all per-BFD locks, like this:
...
bool
gdb_bfd_cache_close_all ()
{
  bool res;
  for (auto abfd : all_bfds)
    {
      auto gdata = static_cast<gdb_bfd_data *> (bfd_usrdata (abfd));
      gdata->per_bfd_mutex.lock ();
    }
  res = bfd_cache_close_all ();
  for (auto abfd : all_bfds)
    {
      auto gdata = static_cast<gdb_bfd_data *> (bfd_usrdata (abfd));
      gdata->per_bfd_mutex.unlock ();
    }
  return res;
}
...

Apart from the fact that trying to hold all those locks at the same time
increases the changes of deadlock, it also accesses all_bfds without locking
the required global BFD lock (reported by TSAN).

It's easy enough to fix that by adding:
...
 gdb_bfd_cache_close_all ()
 {
+  gdb::lock_guard<gdb::recursive_mutex> guard (gdb_bfd_mutex);
...
but that brings us to the problem of lock-order-inversion (also reported by
TSAN), and indeed timeouts do occur.

I came up with a complicated scheme [2] that:
- doesn't try to lock all the per-BFD locks at the same time, and
- addresses the lock-order-inversion problem by releasing the global BFD lock
  before acquiring the per-BFD lock and then re-acquiring the global BFD lock

The scheme is implemented in bfd_cache_close_all:
...
bool
bfd_cache_close_all (per_bfd_lock_unlock_fn_type per_bfd_lock,
		     per_bfd_lock_unlock_fn_type per_bfd_unlock)
{
  bool ret = true;

  if (!bfd_lock ())
    return false;

  while (true)
    {
      bfd *abfd = bfd_last_cache;
      if (abfd == nullptr)
	break;

      /* At this point we'd like to lock the per-bfd lock for abfd, but as it
	 happens we run into lock order inversion problems.  So instead, we
	 unlock the global BFD lock, and then lock the two in the opposite
	 order.  */

      if (!bfd_unlock ())
	return false;

      if (!per_bfd_lock (abfd))
	return false;

      if (!bfd_lock ())
	{
	  per_bfd_unlock (abfd);
	  return false;
	}

      if (abfd != bfd_last_cache)
	{
	  /* While the global BFD lock was briefly unlocked, bfd_last_cache
	     changed, so try again.  */
	  if (!per_bfd_unlock (abfd))
	    {
	      ret = false;
	      break;
	    }
	  continue;
	}

      ret &= _bfd_cache_close_unlocked (abfd);

      if (!per_bfd_unlock (abfd))
	{
	  ret = false;
	  break;
	}

      /* Stop a potential infinite loop should bfd_cache_close()
	 not update bfd_last_cache.  */
      if (bfd_last_cache == abfd)
	break;
    }

  if (!bfd_unlock ())
    return false;

  return ret;
}
...

However, this approach was seen as too convoluted.

So instead, revert to a simple locking scheme with only the global BFD lock,
dropping the per-BFD lock.

This changes the per-BFD locking in gdb_bfd_map_section to global BFD locking,
which means that the read in bfd_get_section_limit_octets is now guarded by
the global BFD lock, which is the same lock guarding the write in
bfd_cache_delete.  So, the race is fixed.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33811

[1] https://sourceware.org/pipermail/gdb-patches/2026-January/224291.html
[2] https://sourceware.org/pipermail/gdb-patches/2026-January/224426.html
---
 gdb/gdb_bfd.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 1933166b5fb..712cea494fa 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -147,17 +147,6 @@ struct gdb_bfd_data
 
   /* The registry.  */
   registry<bfd> registry_fields;
-
-  /* Most of the locking needed for multi-threaded operation is
-     handled by BFD itself.  However, the current BFD model is that
-     locking is only needed for global operations -- but it turned out
-     that the background DWARF reader could race with the auto-load
-     code reading the .debug_gdb_scripts section from the same BFD.
-
-     This lock is the fix: wrappers for important BFD functions will
-     acquire this lock before performing operations that might modify
-     the state of this BFD.  */
-  gdb::mutex per_bfd_mutex;
 };
 
 registry<bfd> *
@@ -766,8 +755,7 @@ gdb_bfd_map_section (asection *sectp, bfd_size_type *size)
 
   abfd = sectp->owner;
 
-  gdb_bfd_data *gdata = (gdb_bfd_data *) bfd_usrdata (abfd);
-  gdb::lock_guard<gdb::mutex> guard (gdata->per_bfd_mutex);
+  gdb::lock_guard<gdb::recursive_mutex> guard (gdb_bfd_mutex);
 
   descriptor = get_section_descriptor (sectp);
 
@@ -1100,8 +1088,7 @@ bool
 gdb_bfd_get_full_section_contents (bfd *abfd, asection *section,
 				   gdb::byte_vector *contents)
 {
-  gdb_bfd_data *gdata = (gdb_bfd_data *) bfd_usrdata (abfd);
-  gdb::lock_guard<gdb::mutex> guard (gdata->per_bfd_mutex);
+  gdb::lock_guard<gdb::recursive_mutex> guard (gdb_bfd_mutex);
 
   bfd_size_type section_size = bfd_get_section_alloc_size (abfd, section);
 
@@ -1116,8 +1103,7 @@ gdb_bfd_get_full_section_contents (bfd *abfd, asection *section,
 int
 gdb_bfd_stat (bfd *abfd, struct stat *sbuf)
 {
-  gdb_bfd_data *gdata = (gdb_bfd_data *) bfd_usrdata (abfd);
-  gdb::lock_guard<gdb::mutex> guard (gdata->per_bfd_mutex);
+  gdb::lock_guard<gdb::recursive_mutex> guard (gdb_bfd_mutex);
 
   return bfd_stat (abfd, sbuf);
 }
@@ -1127,8 +1113,7 @@ gdb_bfd_stat (bfd *abfd, struct stat *sbuf)
 long
 gdb_bfd_get_mtime (bfd *abfd)
 {
-  gdb_bfd_data *gdata = (gdb_bfd_data *) bfd_usrdata (abfd);
-  gdb::lock_guard<gdb::mutex> guard (gdata->per_bfd_mutex);
+  gdb::lock_guard<gdb::recursive_mutex> guard (gdb_bfd_mutex);
 
   return bfd_get_mtime (abfd);
 }
-- 
2.51.0

[PATCH v2 2/2] [gdb/symtab] Fix data race in try_open_dwop_file

[Tom de Vries]

The call to bfd_check_format in try_open_dwop_file:
...
    /* The operations below are not thread-safe, use a lock to synchronize
       concurrent accesses.  */
    static gdb::mutex mutex;
    gdb::lock_guard<gdb::mutex> lock (mutex);

    if (!bfd_check_format (sym_bfd.get (), bfd_object))
      return NULL;
...
accesses the sym_bfd.get () BFD, so it should be guarded by the global BFD
lock.

Fix this by:
- adding a gdb_bfd_check_format that uses the global BFD lock,
- using it in try_open_dwop_file instead of bfd_check_format, and
- removing the local lock.

The local lock also guarded a call to gdb_bfd_record_inclusion, which doesn't
do any locking, so likewise use the global BFD lock in
gdb_bfd_record_inclusion.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33809
---
 gdb/dwarf2/read.c | 15 ++++-----------
 gdb/gdb_bfd.c     | 14 ++++++++++++--
 gdb/gdb_bfd.h     |  4 ++++
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index efe0b046f15..c4979b1e494 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6998,21 +6998,14 @@ try_open_dwop_file (dwarf2_per_bfd *per_bfd, const char *file_name, int is_dwp,
   if (sym_bfd == NULL)
     return NULL;
 
-  {
-    /* The operations below are not thread-safe, use a lock to synchronize
-       concurrent accesses.  */
-    static gdb::mutex mutex;
-    gdb::lock_guard<gdb::mutex> lock (mutex);
-
-    if (!bfd_check_format (sym_bfd.get (), bfd_object))
-      return NULL;
+  if (!gdb_bfd_check_format (sym_bfd.get (), bfd_object))
+    return NULL;
 
-    /* Success.  Record the bfd as having been included by the objfile's bfd.
+  /* Success.  Record the bfd as having been included by the objfile's bfd.
      This is important because things like demangled_names_hash lives in the
      objfile's per_bfd space and may have references to things like symbol
      names that live in the DWO/DWP file's per_bfd space.  PR 16426.  */
-    gdb_bfd_record_inclusion (per_bfd->obfd, sym_bfd.get ());
-  }
+  gdb_bfd_record_inclusion (per_bfd->obfd, sym_bfd.get ());
 
   return sym_bfd;
 }
diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 712cea494fa..bf91b4de155 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -1022,9 +1022,9 @@ gdb_bfd_openr_next_archived_file (bfd *archive, bfd *previous)
 void
 gdb_bfd_record_inclusion (bfd *includer, bfd *includee)
 {
-  struct gdb_bfd_data *gdata;
+  gdb::lock_guard<gdb::recursive_mutex> guard (gdb_bfd_mutex);
 
-  gdata = (struct gdb_bfd_data *) bfd_usrdata (includer);
+  struct gdb_bfd_data *gdata = (struct gdb_bfd_data *) bfd_usrdata (includer);
   gdata->included_bfds.push_back (gdb_bfd_ref_ptr::new_reference (includee));
 }
 
@@ -1196,6 +1196,16 @@ gdb_bfd_canonicalize_symtab (bfd *abfd, bool should_throw)
 			       symbol_table.size () - 1);
 }
 
+/* See gdb_bfd.h.  */
+
+bool
+gdb_bfd_check_format (bfd *abfd, bfd_format format)
+{
+  gdb::lock_guard<gdb::recursive_mutex> guard (gdb_bfd_mutex);
+
+  return bfd_check_format (abfd, format);
+}
+
 /* Implement the 'maint info bfd' command.  */
 
 static void
diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index 69bfa3011aa..2308e76fdd8 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -289,4 +289,8 @@ extern void gdb_bfd_init ();
 extern gdb::array_view<asymbol *> gdb_bfd_canonicalize_symtab
      (bfd *abfd, bool should_throw = true);
 
+/* A wrapper for bfd_check_format that acquires the BFD lock.  */
+
+extern bool gdb_bfd_check_format (bfd *abfd, bfd_format format);
+
 #endif /* GDB_GDB_BFD_H */
-- 
2.51.0

[PING][PATCH v2 0/2] [gdb] TSAN fixes

[Tom de Vries]

On 2/16/26 7:09 PM, Tom de Vries wrote:
> I build gdb with TSAN at -O0, ran the testsuite and found a few problems.
> 
> This series fixes those.
> 
> The first patch changes the locking scheme.
> 
> The second patch removes a local lock in try_open_dwop_file.
> 

Ping.

Thanks,
- Tom

> Changes since v1:
> - commited an approved patch [1]
> - combined two patches related to the local lock in try_open_dwop_file
> - replaced last patch by new patch changing the locking scheme
> 
> [1] https://sourceware.org/pipermail/gdb-patches/2026-January/224422.html
> 
> Tom de Vries (2):
>    [gdb/symtab] Replace per-BFD lock with global BFD lock
>    [gdb/symtab] Fix data race in try_open_dwop_file
> 
>   gdb/dwarf2/read.c | 15 ++++-----------
>   gdb/gdb_bfd.c     | 37 ++++++++++++++++---------------------
>   gdb/gdb_bfd.h     |  4 ++++
>   3 files changed, 24 insertions(+), 32 deletions(-)
> 
> 
> base-commit: 2d306bf8a70e4ef9d6cd6eeecf17a8e6f4f04151

Re: [PATCH v2 1/2] [gdb/symtab] Replace per-BFD lock with global BFD lock

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> A naive way to implement this using the existing scheme of wrappers, would be to
Tom> add a gdb_bfd_cache_close_all that locks all per-BFD locks, calls
Tom> bfd_cache_close_all, and unlocks all per-BFD locks, like this:

I've long considered the BFD cache to be pretty bad.  I am not sure what
would be better but it really doesn't work all that nicely for gdb,
particularly with threads.

Anyway I think this patch is ok.
Approved-By: Tom Tromey <tom@tromey.com>

I do wonder whether this causes any more lock contention.

Tom

Re: [PATCH v2 2/2] [gdb/symtab] Fix data race in try_open_dwop_file

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> The call to bfd_check_format in try_open_dwop_file:
Tom> ...
Tom>     /* The operations below are not thread-safe, use a lock to synchronize
Tom>        concurrent accesses.  */
Tom>     static gdb::mutex mutex;
Tom>     gdb::lock_guard<gdb::mutex> lock (mutex);

Tom>     if (!bfd_check_format (sym_bfd.get (), bfd_object))
Tom>       return NULL;
Tom> ...
Tom> accesses the sym_bfd.get () BFD, so it should be guarded by the global BFD
Tom> lock.

Doesn't bfd_check_format do this itself?
Otherwise, don't we need to wrap all calls to bfd_check_format?

Tom> The local lock also guarded a call to gdb_bfd_record_inclusion, which doesn't
Tom> do any locking, so likewise use the global BFD lock in
Tom> gdb_bfd_record_inclusion.

This part seems unobjectionable to me.

Tom

Re: [PATCH v2 1/2] [gdb/symtab] Replace per-BFD lock with global BFD lock

[Tom de Vries]

On 3/4/26 9:20 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> A naive way to implement this using the existing scheme of wrappers, would be to
> Tom> add a gdb_bfd_cache_close_all that locks all per-BFD locks, calls
> Tom> bfd_cache_close_all, and unlocks all per-BFD locks, like this:
> 
> I've long considered the BFD cache to be pretty bad.  I am not sure what
> would be better but it really doesn't work all that nicely for gdb,
> particularly with threads.
> 
> Anyway I think this patch is ok.
> Approved-By: Tom Tromey <tom@tromey.com>
> 

Thanks for the review, I've pushed this.

BTW, I shortened the commit log a bit, I realized it didn't really need 
the lengthy "complicated scheme" implementation pasted in there, given 
that there's a reference to the corresponding patch.

> I do wonder whether this causes any more lock contention.

Yeah, that seems likely.

Thanks,
- Tom

Re: [PATCH v2 2/2] [gdb/symtab] Fix data race in try_open_dwop_file

[Tom de Vries]

On 3/4/26 9:29 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> The call to bfd_check_format in try_open_dwop_file:
> Tom> ...
> Tom>     /* The operations below are not thread-safe, use a lock to synchronize
> Tom>        concurrent accesses.  */
> Tom>     static gdb::mutex mutex;
> Tom>     gdb::lock_guard<gdb::mutex> lock (mutex);
> 
> Tom>     if (!bfd_check_format (sym_bfd.get (), bfd_object))
> Tom>       return NULL;
> Tom> ...
> Tom> accesses the sym_bfd.get () BFD, so it should be guarded by the global BFD
> Tom> lock.
> 
> Doesn't bfd_check_format do this itself?

Hi Tom,

thanks for the review, and sorry for the delay in replying, I seem to 
have dropped the ball on this.

AFAIU, bfd_check_format does locking, but it's only used to [1]:
...
lock the internal global data structures of BFD (like its global cache 
or some global counter), but not data in individual `bfd *`instances.  ...

So while it does do locking, it's not sufficient.

> Otherwise, don't we need to wrap all calls to bfd_check_format?
> 

Agreed, and likewise for bfd_check_format_matches.

I've submitted a v3 that adds this ( 
https://sourceware.org/pipermail/gdb-patches/2026-May/227652.html ).

Thanks,
- Tom

> Tom> The local lock also guarded a call to gdb_bfd_record_inclusion, which doesn't
> Tom> do any locking, so likewise use the global BFD lock in
> Tom> gdb_bfd_record_inclusion.
> 
> This part seems unobjectionable to me.
> 
> Tom

[1] https://sourceware.org/pipermail/gdb-patches/2026-January/224291.html

Re: [PATCH v2 2/2] [gdb/symtab] Fix data race in try_open_dwop_file

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> AFAIU, bfd_check_format does locking, but it's only used to [1]:
Tom> ...
Tom> lock the internal global data structures of BFD (like its global cache
Tom> or some global counter), but not data in individual `bfd *`instances.
Tom> ...

Tom> So while it does do locking, it's not sufficient.

How so?

I mean, since it's a recursive lock, it's not like locking it some more
can cause a problem.  But at the same time I don't think it adds any
more protection.

Tom

Re: [PATCH v2 2/2] [gdb/symtab] Fix data race in try_open_dwop_file

[Tom de Vries]

On 5/27/26 10:48 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> AFAIU, bfd_check_format does locking, but it's only used to [1]:
> Tom> ...
> Tom> lock the internal global data structures of BFD (like its global cache
> Tom> or some global counter), but not data in individual `bfd *`instances.
> Tom> ...
> 
> Tom> So while it does do locking, it's not sufficient.
> 
> How so?
> 
> I mean, since it's a recursive lock, it's not like locking it some more
> can cause a problem.  But at the same time I don't think it adds any
> more protection.

Well, the race is on fields of individual "bfd *" instances.

More specifically, this write in bfd_cache_delete:
...
   abfd->last_io = bfd_io_force;
...
vs one of these reads (abfd->format or abfd->direction) in:
...
   if (!bfd_read_p (abfd)
       || (unsigned int) abfd->format >= (unsigned int) bfd_type_end)
...

As pointed out by Simon, the bfd code doesn't provide any locking for that.

So the gdb wrappers provide the additional locking needed.  AFAIU, this 
is not specific to these wrappers, all gdb_bfd wrapper are needed for 
the same reason.

Thanks,
- Tom