[binutils-gdb] Add minimal thread-safety to BFD

[binutils-gdb] Add minimal thread-safety to BFD

[Tom Tromey]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1185b5b79a12ba67eb60bee3f75babf7a222fde0

commit 1185b5b79a12ba67eb60bee3f75babf7a222fde0
Author: Tom Tromey <tom@tromey.com>
Date:   Tue Sep 5 19:05:40 2023 -0600

    Add minimal thread-safety to BFD
    
    This patch provides some minimal thread-safety to BFD.
    
    The BFD client can request thread-safety by providing a lock and
    unlock function.  The globals used during BFD creation (e.g.,
    bfd_id_counter) are then locked, and the file descriptor cache is also
    locked.  A function to clean up any thread-local data is now provided
    for BFD clients.
    
            * bfd-in2.h: Regenerate.
            * bfd.c (lock_fn, unlock_fn): New globals.
            (bfd_thread_init, bfd_thread_cleanup, bfd_lock, bfd_unlock): New
            functions.
            * cache.c (bfd_cache_lookup_worker): Use _bfd_open_file_unlocked.
            (cache_btell, cache_bseek, cache_bread, cache_bwrite): Lock
            and unlock.
            (cache_bclose): Add comment.
            (cache_bflush, cache_bstat, cache_bmmap): Lock and unlock.
            (_bfd_cache_init_unlocked): New function.
            (bfd_cache_init): Use it.  Lock and unlock.
            (_bfd_cache_close_unlocked): New function.
            (bfd_cache_close, bfd_cache_close_all): Use it.  Lock and unlock.
            (_bfd_open_file_unlocked): New function.
            (bfd_open_file): Use it.  Lock and unlock.
            * doc/bfd.texi (BFD front end): Add Threading menu item.
            * libbfd.h: Regenerate.
            * opncls.c (_bfd_new_bfd): Lock and unlock.
            * po/bfd.pot: Regenerate.

Diff:
---
 bfd/bfd-in2.h    |   8 +++
 bfd/bfd.c        | 131 ++++++++++++++++++++++++++++++++++++-
 bfd/cache.c      | 193 +++++++++++++++++++++++++++++++++++++++++--------------
 bfd/doc/bfd.texi |   1 +
 bfd/libbfd.h     |   4 ++
 bfd/opncls.c     |   7 ++
 bfd/po/bfd.pot   |  10 +--
 7 files changed, 299 insertions(+), 55 deletions(-)

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 96eef92fdc7..79f7f24436c 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -2572,6 +2572,14 @@ unsigned int bfd_init (void);
 /* Value returned by bfd_init.  */
 #define BFD_INIT_MAGIC (sizeof (struct bfd_section))
 
+typedef bool (*bfd_lock_unlock_fn_type) (void *);
+bool bfd_thread_init
+   (bfd_lock_unlock_fn_type lock,
+    bfd_lock_unlock_fn_type unlock,
+    void *data);
+
+void bfd_thread_cleanup (void);
+
 long bfd_get_reloc_upper_bound (bfd *abfd, asection *sect);
 
 long bfd_canonicalize_reloc
diff --git a/bfd/bfd.c b/bfd/bfd.c
index 2cf8361caa2..99189e0b5ac 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1716,7 +1716,7 @@ bfd_set_assert_handler (bfd_assert_handler_type pnew)
 
 /*
 INODE
-Initialization, Miscellaneous, Error reporting, BFD front end
+Initialization, Threading, Error reporting, BFD front end
 
 FUNCTION
 	bfd_init
@@ -1749,9 +1749,136 @@ bfd_init (void)
   return BFD_INIT_MAGIC;
 }
 \f
+
+/*
+INODE
+Threading, Miscellaneous, Initialization, BFD front end
+
+SECTION
+	Threading
+
+	BFD has limited support for thread-safety.  Most BFD globals
+	are protected by locks, while the error-related globals are
+	thread-local.  A given BFD cannot safely be used from two
+	threads at the same time; it is up to the application to do
+	any needed locking.  However, it is ok for different threads
+	to work on different BFD objects at the same time.
+
+SUBSECTION
+	Thread functions.
+
+CODE_FRAGMENT
+.typedef bool (*bfd_lock_unlock_fn_type) (void *);
+*/
+
+/* The lock and unlock functions, if set.  */
+static bfd_lock_unlock_fn_type lock_fn;
+static bfd_lock_unlock_fn_type unlock_fn;
+static void *lock_data;
+
+/*
+FUNCTION
+	bfd_thread_init
+
+SYNOPSIS
+	bool bfd_thread_init
+	  (bfd_lock_unlock_fn_type lock,
+	  bfd_lock_unlock_fn_type unlock,
+	  void *data);
+
+DESCRIPTION
+
+	Initialize BFD threading.  The functions passed in will be
+	used to lock and unlock global data structures.  This may only
+	be called a single time in a given process.  Returns true on
+	success and false on error.  DATA is passed verbatim to the
+	lock and unlock functions.  The lock and unlock functions
+	should return true on success, or set the BFD error and return
+	false on failure.
+*/
+
+bool
+bfd_thread_init (bfd_lock_unlock_fn_type lock, bfd_lock_unlock_fn_type unlock,
+		 void *data)
+{
+  /* Both functions must be set, and this cannot have been called
+     before.  */
+  if (lock == NULL || unlock == NULL || unlock_fn != NULL)
+    {
+      bfd_set_error (bfd_error_invalid_operation);
+      return false;
+    }
+
+  lock_fn = lock;
+  unlock_fn = unlock;
+  lock_data = data;
+  return true;
+}
+
+/*
+FUNCTION
+	bfd_thread_cleanup
+
+SYNOPSIS
+	void bfd_thread_cleanup (void);
+
+DESCRIPTION
+	Clean up any thread-local state.  This should be called by a
+	thread that uses any BFD functions, before the thread exits.
+	It is fine to call this multiple times, or to call it and then
+	later call BFD functions on the same thread again.
+*/
+
+void
+bfd_thread_cleanup (void)
+{
+  _bfd_clear_error_data ();
+}
+
+/*
+INTERNAL_FUNCTION
+	bfd_lock
+
+SYNOPSIS
+	bool bfd_lock (void);
+
+DESCRIPTION
+	Acquire the global BFD lock, if needed.  Returns true on
+	success, false on error.
+*/
+
+bool
+bfd_lock (void)
+{
+  if (lock_fn != NULL)
+    return lock_fn (lock_data);
+  return true;
+}
+
+/*
+INTERNAL_FUNCTION
+	bfd_unlock
+
+SYNOPSIS
+	bool bfd_unlock (void);
+
+DESCRIPTION
+	Release the global BFD lock, if needed.  Returns true on
+	success, false on error.
+*/
+
+bool
+bfd_unlock (void)
+{
+  if (unlock_fn != NULL)
+    return unlock_fn (lock_data);
+  return true;
+}
+
+
 /*
 INODE
-Miscellaneous, Memory Usage, Initialization, BFD front end
+Miscellaneous, Memory Usage, Threading, BFD front end
 
 SECTION
 	Miscellaneous
diff --git a/bfd/cache.c b/bfd/cache.c
index 3d26bee0773..a392bd28e18 100644
--- a/bfd/cache.c
+++ b/bfd/cache.c
@@ -49,6 +49,8 @@ SUBSECTION
 #include <sys/mman.h>
 #endif
 
+static FILE *_bfd_open_file_unlocked (bfd *abfd);
+
 /* In some cases we can optimize cache operation when reopening files.
    For instance, a flush is entirely unnecessary if the file is already
    closed, so a flush would use CACHE_NO_OPEN.  Similarly, a seek using
@@ -259,7 +261,7 @@ bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag)
   if (flag & CACHE_NO_OPEN)
     return NULL;
 
-  if (bfd_open_file (abfd) == NULL)
+  if (_bfd_open_file_unlocked (abfd) == NULL)
     ;
   else if (!(flag & CACHE_NO_SEEK)
 	   && _bfd_real_fseek ((FILE *) abfd->iostream,
@@ -278,19 +280,36 @@ bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag)
 static file_ptr
 cache_btell (struct bfd *abfd)
 {
+  if (!bfd_lock ())
+    return -1;
   FILE *f = bfd_cache_lookup (abfd, CACHE_NO_OPEN);
   if (f == NULL)
-    return abfd->where;
-  return _bfd_real_ftell (f);
+    {
+      if (!bfd_unlock ())
+	return -1;
+      return abfd->where;
+    }
+  file_ptr result = _bfd_real_ftell (f);
+  if (!bfd_unlock ())
+    return -1;
+  return result;
 }
 
 static int
 cache_bseek (struct bfd *abfd, file_ptr offset, int whence)
 {
+  if (!bfd_lock ())
+    return -1;
   FILE *f = bfd_cache_lookup (abfd, whence != SEEK_CUR ? CACHE_NO_SEEK : CACHE_NORMAL);
   if (f == NULL)
+    {
+      bfd_unlock ();
+      return -1;
+    }
+  int result = _bfd_real_fseek (f, offset, whence);
+  if (!bfd_unlock ())
     return -1;
-  return _bfd_real_fseek (f, offset, whence);
+  return result;
 }
 
 /* Note that archive entries don't have streams; they share their parent's.
@@ -338,12 +357,17 @@ cache_bread_1 (FILE *f, void *buf, file_ptr nbytes)
 static file_ptr
 cache_bread (struct bfd *abfd, void *buf, file_ptr nbytes)
 {
+  if (!bfd_lock ())
+    return -1;
   file_ptr nread = 0;
   FILE *f;
 
   f = bfd_cache_lookup (abfd, CACHE_NORMAL);
   if (f == NULL)
-    return -1;
+    {
+      bfd_unlock ();
+      return -1;
+    }
 
   /* Some filesystems are unable to handle reads that are too large
      (for instance, NetApp shares with oplocks turned off).  To avoid
@@ -374,57 +398,84 @@ cache_bread (struct bfd *abfd, void *buf, file_ptr nbytes)
 	break;
     }
 
+  if (!bfd_unlock ())
+    return -1;
   return nread;
 }
 
 static file_ptr
 cache_bwrite (struct bfd *abfd, const void *from, file_ptr nbytes)
 {
+  if (!bfd_lock ())
+    return -1;
   file_ptr nwrite;
   FILE *f = bfd_cache_lookup (abfd, CACHE_NORMAL);
 
   if (f == NULL)
-    return 0;
+    {
+      if (!bfd_unlock ())
+	return -1;
+      return 0;
+    }
   nwrite = fwrite (from, 1, nbytes, f);
   if (nwrite < nbytes && ferror (f))
     {
       bfd_set_error (bfd_error_system_call);
+      bfd_unlock ();
       return -1;
     }
+  if (!bfd_unlock ())
+    return -1;
   return nwrite;
 }
 
 static int
 cache_bclose (struct bfd *abfd)
 {
+  /* No locking needed here, it's handled by the callee.  */
   return bfd_cache_close (abfd) - 1;
 }
 
 static int
 cache_bflush (struct bfd *abfd)
 {
+  if (!bfd_lock ())
+    return -1;
   int sts;
   FILE *f = bfd_cache_lookup (abfd, CACHE_NO_OPEN);
 
   if (f == NULL)
-    return 0;
+    {
+      if (!bfd_unlock ())
+	return -1;
+      return 0;
+    }
   sts = fflush (f);
   if (sts < 0)
     bfd_set_error (bfd_error_system_call);
+  if (!bfd_unlock ())
+    return -1;
   return sts;
 }
 
 static int
 cache_bstat (struct bfd *abfd, struct stat *sb)
 {
+  if (!bfd_lock ())
+    return -1;
   int sts;
   FILE *f = bfd_cache_lookup (abfd, CACHE_NO_SEEK_ERROR);
 
   if (f == NULL)
-    return -1;
+    {
+      bfd_unlock ();
+      return -1;
+    }
   sts = fstat (fileno (f), sb);
   if (sts < 0)
     bfd_set_error (bfd_error_system_call);
+  if (!bfd_unlock ())
+    return -1;
   return sts;
 }
 
@@ -440,6 +491,8 @@ cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
 {
   void *ret = (void *) -1;
 
+  if (!bfd_lock ())
+    return ret;
   if ((abfd->flags & BFD_IN_MEMORY) != 0)
     abort ();
 #ifdef HAVE_MMAP
@@ -452,7 +505,10 @@ cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
 
       f = bfd_cache_lookup (abfd, CACHE_NO_SEEK_ERROR);
       if (f == NULL)
-	return ret;
+	{
+	  bfd_unlock ();
+	  return ret;
+	}
 
       if (pagesize_m1 == 0)
 	pagesize_m1 = getpagesize () - 1;
@@ -473,6 +529,8 @@ cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
     }
 #endif
 
+  if (!bfd_unlock ())
+    return (void *) -1;
   return ret;
 }
 
@@ -482,6 +540,22 @@ static const struct bfd_iovec cache_iovec =
   &cache_bclose, &cache_bflush, &cache_bstat, &cache_bmmap
 };
 
+static bool
+_bfd_cache_init_unlocked (bfd *abfd)
+{
+  BFD_ASSERT (abfd->iostream != NULL);
+  if (open_files >= bfd_cache_max_open ())
+    {
+      if (! close_one ())
+	return false;
+    }
+  abfd->iovec = &cache_iovec;
+  insert (abfd);
+  abfd->flags &= ~BFD_CLOSED_BY_CACHE;
+  ++open_files;
+  return true;
+}
+
 /*
 INTERNAL_FUNCTION
 	bfd_cache_init
@@ -496,17 +570,28 @@ DESCRIPTION
 bool
 bfd_cache_init (bfd *abfd)
 {
-  BFD_ASSERT (abfd->iostream != NULL);
-  if (open_files >= bfd_cache_max_open ())
-    {
-      if (! close_one ())
-	return false;
-    }
-  abfd->iovec = &cache_iovec;
-  insert (abfd);
-  abfd->flags &= ~BFD_CLOSED_BY_CACHE;
-  ++open_files;
-  return true;
+  if (!bfd_lock ())
+    return false;
+  bool result = _bfd_cache_init_unlocked (abfd);
+  if (!bfd_unlock ())
+    return false;
+  return result;
+}
+
+static bool
+_bfd_cache_close_unlocked (bfd *abfd)
+{
+  /* Don't remove this test.  bfd_reinit depends on it.  */
+  if (abfd->iovec != &cache_iovec)
+    return true;
+
+  if (abfd->iostream == NULL)
+    /* Previously closed.  */
+    return true;
+
+  /* Note: no locking needed in this function, as it is handled by
+     bfd_cache_delete.  */
+  return bfd_cache_delete (abfd);
 }
 
 /*
@@ -527,15 +612,12 @@ DESCRIPTION
 bool
 bfd_cache_close (bfd *abfd)
 {
-  /* Don't remove this test.  bfd_reinit depends on it.  */
-  if (abfd->iovec != &cache_iovec)
-    return true;
-
-  if (abfd->iostream == NULL)
-    /* Previously closed.  */
-    return true;
-
-  return bfd_cache_delete (abfd);
+  if (!bfd_lock ())
+    return false;
+  bool result = _bfd_cache_close_unlocked (abfd);
+  if (!bfd_unlock ())
+    return false;
+  return result;
 }
 
 /*
@@ -560,11 +642,13 @@ bfd_cache_close_all (void)
 {
   bool ret = true;
 
+  if (!bfd_lock ())
+    return false;
   while (bfd_last_cache != NULL)
     {
       bfd *prev_bfd_last_cache = bfd_last_cache;
 
-      ret &= bfd_cache_close (bfd_last_cache);
+      ret &= _bfd_cache_close_unlocked (bfd_last_cache);
 
       /* Stop a potential infinite loop should bfd_cache_close()
 	 not update bfd_last_cache.  */
@@ -572,6 +656,8 @@ bfd_cache_close_all (void)
 	break;
     }
 
+  if (!bfd_unlock ())
+    return false;
   return ret;
 }
 
@@ -592,23 +678,8 @@ bfd_cache_size (void)
   return open_files;
 }
 
-/*
-INTERNAL_FUNCTION
-	bfd_open_file
-
-SYNOPSIS
-	FILE* bfd_open_file (bfd *abfd);
-
-DESCRIPTION
-	Call the OS to open a file for @var{abfd}.  Return the <<FILE *>>
-	(possibly <<NULL>>) that results from this operation.  Set up the
-	BFD so that future accesses know the file is open. If the <<FILE *>>
-	returned is <<NULL>>, then it won't have been put in the
-	cache, so it won't have to be removed from it.
-*/
-
-FILE *
-bfd_open_file (bfd *abfd)
+static FILE *
+_bfd_open_file_unlocked (bfd *abfd)
 {
   abfd->cacheable = true;	/* Allow it to be closed later.  */
 
@@ -673,9 +744,35 @@ bfd_open_file (bfd *abfd)
     bfd_set_error (bfd_error_system_call);
   else
     {
-      if (! bfd_cache_init (abfd))
+      if (! _bfd_cache_init_unlocked (abfd))
 	return NULL;
     }
 
   return (FILE *) abfd->iostream;
 }
+
+/*
+INTERNAL_FUNCTION
+	bfd_open_file
+
+SYNOPSIS
+	FILE* bfd_open_file (bfd *abfd);
+
+DESCRIPTION
+	Call the OS to open a file for @var{abfd}.  Return the <<FILE *>>
+	(possibly <<NULL>>) that results from this operation.  Set up the
+	BFD so that future accesses know the file is open. If the <<FILE *>>
+	returned is <<NULL>>, then it won't have been put in the
+	cache, so it won't have to be removed from it.
+*/
+
+FILE *
+bfd_open_file (bfd *abfd)
+{
+  if (!bfd_lock ())
+    return NULL;
+  FILE *result = _bfd_open_file_unlocked (abfd);
+  if (!bfd_unlock ())
+    return NULL;
+  return result;
+}
diff --git a/bfd/doc/bfd.texi b/bfd/doc/bfd.texi
index f348710845f..3f70cb73a1d 100644
--- a/bfd/doc/bfd.texi
+++ b/bfd/doc/bfd.texi
@@ -199,6 +199,7 @@ IEEE-695.
 * typedef bfd::
 * Error reporting::
 * Initialization::
+* Threading::
 * Miscellaneous::
 * Memory Usage::
 * Sections::
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index 498ce8d95b5..cc432677a81 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -937,6 +937,10 @@ bfd_error_handler_type _bfd_set_error_handler_caching (bfd *) ATTRIBUTE_HIDDEN;
 
 const char *_bfd_get_error_program_name (void) ATTRIBUTE_HIDDEN;
 
+bool bfd_lock (void) ATTRIBUTE_HIDDEN;
+
+bool bfd_unlock (void) ATTRIBUTE_HIDDEN;
+
 /* Extracted from bfdio.c.  */
 struct bfd_iovec
 {
diff --git a/bfd/opncls.c b/bfd/opncls.c
index cddfd7ec1fb..5a77562744c 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -81,6 +81,8 @@ _bfd_new_bfd (void)
   if (nbfd == NULL)
     return NULL;
 
+  if (!bfd_lock ())
+    return NULL;
   if (bfd_use_reserved_id)
     {
       nbfd->id = --bfd_reserved_id_counter;
@@ -88,6 +90,11 @@ _bfd_new_bfd (void)
     }
   else
     nbfd->id = bfd_id_counter++;
+  if (!bfd_unlock ())
+    {
+      free (nbfd);
+      return NULL;
+    }
 
   nbfd->memory = objalloc_create ();
   if (nbfd->memory == NULL)
diff --git a/bfd/po/bfd.pot b/bfd/po/bfd.pot
index 6c979791ef7..bc428dad3b2 100644
--- a/bfd/po/bfd.pot
+++ b/bfd/po/bfd.pot
@@ -231,22 +231,22 @@ msgstr ""
 msgid "#<invalid error code>"
 msgstr ""
 
-#: bfd.c:1892
+#: bfd.c:2019
 #, c-format
 msgid "BFD %s assertion fail %s:%d"
 msgstr ""
 
-#: bfd.c:1905
+#: bfd.c:2032
 #, c-format
 msgid "BFD %s internal error, aborting at %s:%d in %s\n"
 msgstr ""
 
-#: bfd.c:1910
+#: bfd.c:2037
 #, c-format
 msgid "BFD %s internal error, aborting at %s:%d\n"
 msgstr ""
 
-#: bfd.c:1912
+#: bfd.c:2039
 msgid "Please report this bug.\n"
 msgstr ""
 
@@ -265,7 +265,7 @@ msgstr ""
 msgid "warning: writing section `%pA' at huge (ie negative) file offset"
 msgstr ""
 
-#: cache.c:273
+#: cache.c:275
 #, c-format
 msgid "reopening %pB: %s"
 msgstr ""