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

[Tom de Vries <tdevries@suse.de>]

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