[3/3] Add minimal thread-safety to BFD

Message ID 20231026191435.204144-4-tom@tromey.com
State New
Headers
Series Threads and BFD |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed

Commit Message

Tom Tromey Oct. 26, 2023, 7:07 p.m. UTC
  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.
---
 bfd/bfd-in2.h    |   6 ++
 bfd/bfd.c        | 109 +++++++++++++++++++++++++++++-
 bfd/cache.c      | 170 +++++++++++++++++++++++++++++++++--------------
 bfd/doc/bfd.texi |   1 +
 bfd/libbfd.h     |   4 ++
 bfd/opncls.c     |   2 +
 bfd/po/bfd.pot   |  10 +--
 7 files changed, 246 insertions(+), 56 deletions(-)
  

Comments

Nick Clifton Nov. 1, 2023, 12:45 p.m. UTC | #1
Hi Tom,

> 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.

I have one concern about this patch:


> +typedef void (*bfd_lock_unlock_fn_type) (void);

Given that we are dealing with client provided locking and unlocking
functions, I feel that the client might want to be able to reference
a data structure of their own performing these actions.  (I am not
hugely familiar with locking and unlocking functions, so maybe I am
mistaken here).  I also wonder if the functions might be interested
in the BFD being locked.  Thus I think that the typedef might be
better specified as:

  typedef void (* bfd_lock_unlock_fn_type) (bfd *, void *);

I also wonder if the functions should be allowed to fail, in which
case the typedef would be:

  typedef bool (* bfd_lock_unlock_fn_type) (bfd *, void *);

Given all of that the init function would have to be extended to take
a data pointer:

> +void
> +bfd_thread_init (bfd_lock_unlock_fn_type lock, 
                     bfd_lock_unlock_fn_type unlock,
                     void * data)
> +{
> +  lock_fn = lock;
> +  unlock_fn = unlock;
      lock_data = data;
> +}

Also - should this function let the caller know if a previous set
of lock/unlock functions had been registered, or if there was a problem
registering them ?  (For example is it OK to have a lock function but
not an unlock function ?)  ie:

   bool
   bfd_thread_init (bfd_lock_unlock_fn_type lock,
                    bfd_lock_unlock_fn_type unlock,
                    void * data)
   {
      bool ret = lock_fn == NULL && unlock_fn == NULL;

      lock_fn = lock;
      unlock_fn = unlock;
      lock_data = data;

      ret &= (lock_fn != NULL && unlock_fn != NULL) || (lock_fn == NULL && unlock_fn == NULL);
      return ret;
   }


And of course the bfd_lock and bfd_unlock would need to be updated as well, eg:

> +bool
> +bfd_lock (bfd * abfd)
> +{
> +  if (lock_fn != NULL)
> +    return lock_fn (abfd, lock_data);
      return true;
> +}

What do you think ?

Cheers
   Nick
  
Tom Tromey Nov. 1, 2023, 8:08 p.m. UTC | #2
>>>>> "Nick" == Nick Clifton <nickc@redhat.com> writes:

>> +typedef void (*bfd_lock_unlock_fn_type) (void);

Nick> Given that we are dealing with client provided locking and unlocking
Nick> functions, I feel that the client might want to be able to reference
Nick> a data structure of their own performing these actions.  (I am not
Nick> hugely familiar with locking and unlocking functions, so maybe I am
Nick> mistaken here).  I also wonder if the functions might be interested
Nick> in the BFD being locked.  Thus I think that the typedef might be
Nick> better specified as:

Nick>  typedef void (* bfd_lock_unlock_fn_type) (bfd *, void *);

Seems reasonable.

Nick> I also wonder if the functions should be allowed to fail, in which
Nick> case the typedef would be:

Nick>  typedef bool (* bfd_lock_unlock_fn_type) (bfd *, void *);

I suppose the BFD callers can just return if these fail.

Nick> Also - should this function let the caller know if a previous set
Nick> of lock/unlock functions had been registered, or if there was a problem
Nick> registering them ?  (For example is it OK to have a lock function but
Nick> not an unlock function ?)  ie:

Yeah, I've changed it to fail if (1) it's ever been called before (I
don't think it's really necessary to support this right now) and (2) if
one of the functions is not specified.

Tom
  
Tom Tromey Nov. 1, 2023, 8:48 p.m. UTC | #3
Nick> typedef bool (* bfd_lock_unlock_fn_type) (bfd *, void *);

Tom> I suppose the BFD callers can just return if these fail.

Well, I started on this change, but I am not sure about it now.

cache_btell doesn't seem to have any way to even report an error.  So,
I'm not sure what to do there.  Ignoring the failure is no good.

Or, consider things like cache_bseek.  Handling a failure of bfd_lock is
easy, but there's an unlock at the end:

  int result = _bfd_real_fseek (f, offset, whence);
  bfd_unlock ();
  return result;

If that fails should this really return -1?  Similarly for cache_bwrite,
etc.

On the other hand, I don't think these things really can fail.  Like,
pthread_mutex_lock/unlock failures modes all are basically programming
errors.  So this may argue for just "doing anything".  Or we could
declare these to be infallible.

Let me know what you'd prefer.

Tom
  
Nick Clifton Nov. 2, 2023, 12:14 p.m. UTC | #4
Hi Tom,

> Nick> typedef bool (* bfd_lock_unlock_fn_type) (bfd *, void *);
> 
> Tom> I suppose the BFD callers can just return if these fail.
> 
> Well, I started on this change, but I am not sure about it now.
> 
> cache_btell doesn't seem to have any way to even report an error.  So,
> I'm not sure what to do there.  Ignoring the failure is no good.

It could return "(ufile_ptr) -1".  This would be somewhat inline with
how ftell() behaves.


> Or, consider things like cache_bseek.  Handling a failure of bfd_lock is
> easy, but there's an unlock at the end:
> 
>    int result = _bfd_real_fseek (f, offset, whence);
>    bfd_unlock ();
>    return result;
> 
> If that fails should this really return -1?

Yes.

If the unlock has failed then something pretty bad must have happened and
it would not be a good idea to let the code carry on as if everything was
OK.


   Similarly for cache_bwrite,
> etc.

Yes - the same for these - if the lock or unlock fail then we should assume
the worst and try to return to the client as cleanly as possible.


> On the other hand, I don't think these things really can fail.  Like,
> pthread_mutex_lock/unlock failures modes all are basically programming
> errors.  So this may argue for just "doing anything".  Or we could
> declare these to be infallible.

I agree that they are unlikely to fail.  But being paranoid never hurts.
Since the caller is implementing the locks they can always choose to
just return "true" no matter what.  But if we do not support a failure
mode then one day someone, somewhere is bound to complain...


> Let me know what you'd prefer.

Be safe, be paranoid, and let the client decide whether their code can or
cannot fail.

Cheers
   Nick
  
Tom Tromey Nov. 3, 2023, 11:10 p.m. UTC | #5
>> Let me know what you'd prefer.

Nick> Be safe, be paranoid, and let the client decide whether their code can or
Nick> cannot fail.

Alright.  I'll send v2 shortly.
I didn't check in the 2 patches you approved since I felt they didn't
really make sense in isolation.

Tom
  

Patch

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index e26bc40a9e1..13c4207667a 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -2572,6 +2572,12 @@  unsigned int bfd_init (void);
 /* Value returned by bfd_init.  */
 #define BFD_INIT_MAGIC (sizeof (struct bfd_section))
 
+typedef void (*bfd_lock_unlock_fn_type) (void);
+void bfd_thread_init
+   (bfd_lock_unlock_fn_type lock, bfd_lock_unlock_fn_type unlock);
+
+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..18aa21884fd 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,114 @@  bfd_init (void)
   return BFD_INIT_MAGIC;
 }
 
+
+/*
+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 void (*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;
+
+/*
+FUNCTION
+	bfd_thread_init
+
+SYNOPSIS
+	void bfd_thread_init
+	  (bfd_lock_unlock_fn_type lock, bfd_lock_unlock_fn_type unlock);
+
+DESCRIPTION
+
+	Initialize BFD threading.  The functions passed in will be
+	used to lock and unlock global data structures.
+*/
+
+void
+bfd_thread_init (bfd_lock_unlock_fn_type lock, bfd_lock_unlock_fn_type unlock)
+{
+  lock_fn = lock;
+  unlock_fn = unlock;
+}
+
+/*
+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
+	void bfd_lock (void);
+
+DESCRIPTION
+	Acquire the global BFD lock, if needed.
+*/
+
+void
+bfd_lock (void)
+{
+  if (lock_fn != NULL)
+    lock_fn ();
+}
+
+/*
+INTERNAL_FUNCTION
+	bfd_unlock
+
+SYNOPSIS
+	void bfd_unlock (void);
+
+DESCRIPTION
+	Release the global BFD lock, if needed.
+*/
+
+void
+bfd_unlock (void)
+{
+  if (unlock_fn != NULL)
+    unlock_fn ();
+}
+
+
 /*
 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..ae7ebcdadd6 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,31 @@  bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag)
 static file_ptr
 cache_btell (struct bfd *abfd)
 {
+  bfd_lock ();
   FILE *f = bfd_cache_lookup (abfd, CACHE_NO_OPEN);
   if (f == NULL)
-    return abfd->where;
-  return _bfd_real_ftell (f);
+    {
+      bfd_unlock ();
+      return abfd->where;
+    }
+  file_ptr result = _bfd_real_ftell (f);
+  bfd_unlock ();
+  return result;
 }
 
 static int
 cache_bseek (struct bfd *abfd, file_ptr offset, int whence)
 {
+  bfd_lock ();
   FILE *f = bfd_cache_lookup (abfd, whence != SEEK_CUR ? CACHE_NO_SEEK : CACHE_NORMAL);
   if (f == NULL)
-    return -1;
-  return _bfd_real_fseek (f, offset, whence);
+    {
+      bfd_unlock ();
+      return -1;
+    }
+  int result = _bfd_real_fseek (f, offset, whence);
+  bfd_unlock ();
+  return result;
 }
 
 /* Note that archive entries don't have streams; they share their parent's.
@@ -338,12 +352,16 @@  cache_bread_1 (FILE *f, void *buf, file_ptr nbytes)
 static file_ptr
 cache_bread (struct bfd *abfd, void *buf, file_ptr nbytes)
 {
+  bfd_lock ();
   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 +392,75 @@  cache_bread (struct bfd *abfd, void *buf, file_ptr nbytes)
 	break;
     }
 
+  bfd_unlock ();
   return nread;
 }
 
 static file_ptr
 cache_bwrite (struct bfd *abfd, const void *from, file_ptr nbytes)
 {
+  bfd_lock ();
   file_ptr nwrite;
   FILE *f = bfd_cache_lookup (abfd, CACHE_NORMAL);
 
   if (f == NULL)
-    return 0;
+    {
+      bfd_unlock ();
+      return 0;
+    }
   nwrite = fwrite (from, 1, nbytes, f);
   if (nwrite < nbytes && ferror (f))
     {
       bfd_set_error (bfd_error_system_call);
+      bfd_unlock ();
       return -1;
     }
+  bfd_unlock ();
   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)
 {
+  bfd_lock ();
   int sts;
   FILE *f = bfd_cache_lookup (abfd, CACHE_NO_OPEN);
 
   if (f == NULL)
-    return 0;
+    {
+      bfd_unlock ();
+      return 0;
+    }
   sts = fflush (f);
   if (sts < 0)
     bfd_set_error (bfd_error_system_call);
+  bfd_unlock ();
   return sts;
 }
 
 static int
 cache_bstat (struct bfd *abfd, struct stat *sb)
 {
+  bfd_lock ();
   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);
+  bfd_unlock ();
   return sts;
 }
 
@@ -440,6 +476,7 @@  cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
 {
   void *ret = (void *) -1;
 
+  bfd_lock ();
   if ((abfd->flags & BFD_IN_MEMORY) != 0)
     abort ();
 #ifdef HAVE_MMAP
@@ -452,7 +489,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 +513,7 @@  cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED,
     }
 #endif
 
+  bfd_unlock ();
   return ret;
 }
 
@@ -482,6 +523,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 +553,26 @@  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;
+  bfd_lock ();
+  bool result = _bfd_cache_init_unlocked (abfd);
+  bfd_unlock ();
+  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 +593,10 @@  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);
+  bfd_lock ();
+  bool result = _bfd_cache_close_unlocked (abfd);
+  bfd_unlock ();
+  return result;
 }
 
 /*
@@ -560,11 +621,12 @@  bfd_cache_close_all (void)
 {
   bool ret = true;
 
+  bfd_lock ();
   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 +634,7 @@  bfd_cache_close_all (void)
 	break;
     }
 
+  bfd_unlock ();
   return ret;
 }
 
@@ -592,23 +655,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 +721,33 @@  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)
+{
+  bfd_lock ();
+  FILE *result = _bfd_open_file_unlocked (abfd);
+  bfd_unlock ();
+  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 fea00b6ee45..54b502c2dd7 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;
 
+void bfd_lock (void) ATTRIBUTE_HIDDEN;
+
+void bfd_unlock (void) ATTRIBUTE_HIDDEN;
+
 /* Extracted from bfdio.c.  */
 struct bfd_iovec
 {
diff --git a/bfd/opncls.c b/bfd/opncls.c
index cddfd7ec1fb..ae04821405e 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -81,6 +81,7 @@  _bfd_new_bfd (void)
   if (nbfd == NULL)
     return NULL;
 
+  bfd_lock ();
   if (bfd_use_reserved_id)
     {
       nbfd->id = --bfd_reserved_id_counter;
@@ -88,6 +89,7 @@  _bfd_new_bfd (void)
     }
   else
     nbfd->id = bfd_id_counter++;
+  bfd_unlock ();
 
   nbfd->memory = objalloc_create ();
   if (nbfd->memory == NULL)
diff --git a/bfd/po/bfd.pot b/bfd/po/bfd.pot
index 33d51490482..ed637fcafe9 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:1997
 #, c-format
 msgid "BFD %s assertion fail %s:%d"
 msgstr ""
 
-#: bfd.c:1905
+#: bfd.c:2010
 #, c-format
 msgid "BFD %s internal error, aborting at %s:%d in %s\n"
 msgstr ""
 
-#: bfd.c:1910
+#: bfd.c:2015
 #, c-format
 msgid "BFD %s internal error, aborting at %s:%d\n"
 msgstr ""
 
-#: bfd.c:1912
+#: bfd.c:2017
 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 ""