[PATCH 1/2] Thread-safety improvements for bfd_check_format_matches

[Tom Tromey <tom@tromey.com>]

A gdb bug found that bfd_check_format_matches has some data races when
called from multiple threads.

In particular, it changes the BFD error handler, which is a global.
It also has a local static variable ("in_check_format") that is used
for recursion detection.  And, finally, it may emit warnings to the
per-xvec warning array, which is a global.

This patch removes all the races here.  I believe it does not change
the current semantics, but I was unable to get the test file that
would let me test that.

The first part of patch is to change _bfd_error_handler to directly
handle the needs of bfd_check_format_matches.  This way, the error
handler does not need to be changed.

This change lets us use the new per-thread global
(error_handler_messages, replacing error_handler_bfd) to also remove
the need for in_check_format -- a single variable suffices.

Finally, the global per-xvec array is replaced with a new type that
holds the error messages.  The outermost such type is stack-allocated
in bfd_check_format_matches.

I tested this using the binutils test suite.  I also built gdb with
thread sanitizer and ran the test case that was noted as failing.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31264
---
 bfd/bfd.c     | 68 ++++++++++++++++++++++++++++++++++++++-----------
 bfd/format.c  | 70 +++++++++++++++++++++++----------------------------
 bfd/libbfd.h  | 17 +++++++++++--
 bfd/targets.c | 59 ++++++++++++++++++++++++++++++-------------
 4 files changed, 142 insertions(+), 72 deletions(-)

diff --git a/bfd/bfd.c b/bfd/bfd.c
index 54061a34240..11ad7f24456 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1534,10 +1534,17 @@ err_sprintf (void *stream, const char *fmt, ...)
   return total;
 }
 
-/* Communicate the bfd processed by bfd_check_format_matches to the
-   error handling function error_handler_sprintf.  */
+/* Communicate the error-message container processed by
+   bfd_check_format_matches to the error handling function
+   error_handler_sprintf.  When non-NULL, _bfd_error_handler will call
+   error_handler_sprintf; when NULL, _bfd_error_internal will be used
+   instead.  */
 
-static bfd *error_handler_bfd;
+static TLS struct per_xvec_messages *error_handler_messages;
+
+/* A special value for error_handler_messages that indicates that the
+   error should simply be ignored.  */
+#define IGNORE_ERROR_MESSAGES ((struct per_xvec_messages *) -1)
 
 /* An error handler that prints to a string, then dups that string to
    a per-xvec cache.  */
@@ -1554,12 +1561,12 @@ error_handler_sprintf (const char *fmt, va_list ap)
   _bfd_print (err_sprintf, &error_stream, fmt, ap);
 
   size_t len = error_stream.ptr - error_buf;
-  struct per_xvec_message **warn
-    = _bfd_per_xvec_warn (error_handler_bfd->xvec, len + 1);
-  if (*warn)
+  struct per_xvec_message *warn
+    = _bfd_per_xvec_warn (error_handler_messages, len + 1);
+  if (warn)
     {
-      memcpy ((*warn)->message, error_buf, len);
-      (*warn)->message[len] = 0;
+      memcpy (warn->message, error_buf, len);
+      warn->message[len] = 0;
     }
 }
 
@@ -1597,7 +1604,14 @@ _bfd_error_handler (const char *fmt, ...)
   va_list ap;
 
   va_start (ap, fmt);
-  _bfd_error_internal (fmt, ap);
+  if (error_handler_messages == IGNORE_ERROR_MESSAGES)
+    {
+      /* Nothing.  */
+    }
+  else if (error_handler_messages != NULL)
+    error_handler_sprintf (fmt, ap);
+  else
+    _bfd_error_internal (fmt, ap);
   va_end (ap);
 }
 
@@ -1628,18 +1642,42 @@ INTERNAL_FUNCTION
 	_bfd_set_error_handler_caching
 
 SYNOPSIS
-	bfd_error_handler_type _bfd_set_error_handler_caching (bfd *);
+	struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *);
 
 DESCRIPTION
 	Set the BFD error handler function to one that stores messages
-	to the per_xvec_warn array.  Returns the previous function.
+	to the per_xvec_warn array.  Returns the previous BFD to which
+	messages are stored.  Note that two sequential calls to this
+	with a non-NULL BFD will cause output to be dropped, rather
+	than gathered.
 */
 
-bfd_error_handler_type
-_bfd_set_error_handler_caching (bfd *abfd)
+struct per_xvec_messages *
+_bfd_set_error_handler_caching (struct per_xvec_messages *messages)
+{
+  struct per_xvec_messages *old = error_handler_messages;
+  if (old == NULL)
+    error_handler_messages = messages;
+  else
+    error_handler_messages = IGNORE_ERROR_MESSAGES;
+  return old;
+}
+
+/*
+INTERNAL_FUNCTION
+	_bfd_restore_error_handler_caching
+
+SYNOPSIS
+	void _bfd_restore_error_handler_caching (struct per_xvec_messages *);
+
+DESCRIPTION
+	Reset the BFD error handler function an earlier value.
+*/
+
+void
+_bfd_restore_error_handler_caching (struct per_xvec_messages *old)
 {
-  error_handler_bfd = abfd;
-  return bfd_set_error_handler (error_handler_sprintf);
+  error_handler_messages = old;
 }
 
 /*
diff --git a/bfd/format.c b/bfd/format.c
index 8f3fc7e7b96..5a5b01975ac 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -272,10 +272,33 @@ clear_warnmsg (struct per_xvec_message **list)
   *list = NULL;
 }
 
+/* Free all the storage in LIST.  Note that the first element of LIST
+   is special and is assumed to be stack-allocated.  TARG is used for
+   re-issuing warning messages.  If TARG is non-NULL, then any
+   messages in LIST with a matching target are issued.  If TARG is
+   NULL, then messages are only reissued if they are all associated
+   with a single BFD target.  */
+
 static void
-null_error_handler (const char *fmt ATTRIBUTE_UNUSED,
-		    va_list ap ATTRIBUTE_UNUSED)
+print_and_clear_messages (struct per_xvec_messages *list,
+			  const bfd_target *targ)
 {
+  struct per_xvec_messages *iter = list;
+
+  if (targ == NULL && list->next == NULL)
+    print_warnmsg (&list->messages);
+
+  while (iter != NULL)
+    {
+      struct per_xvec_messages *next = iter->next;
+
+      if (iter->targ == targ)
+	print_warnmsg (&iter->messages);
+      clear_warnmsg (&iter->messages);
+      if (iter != list)
+	free (iter);
+      iter = next;
+    }
 }
 
 /*
@@ -313,8 +336,8 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   unsigned int initial_section_id = _bfd_section_id;
   struct bfd_preserve preserve, preserve_match;
   bfd_cleanup cleanup = NULL;
-  bfd_error_handler_type orig_error_handler;
-  static int in_check_format;
+  struct per_xvec_messages messages = { abfd, NULL, NULL, NULL };
+  struct per_xvec_messages *orig_messages;
 
   if (matching != NULL)
     *matching = NULL;
@@ -345,11 +368,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
 
   /* Don't report errors on recursive calls checking the first element
      of an archive.  */
-  if (in_check_format)
-    orig_error_handler = bfd_set_error_handler (null_error_handler);
-  else
-    orig_error_handler = _bfd_set_error_handler_caching (abfd);
-  ++in_check_format;
+  orig_messages = _bfd_set_error_handler_caching (&messages);
 
   preserve_match.marker = NULL;
   if (!bfd_preserve_save (abfd, &preserve, NULL))
@@ -591,15 +610,9 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
       if (preserve_match.marker != NULL)
 	bfd_preserve_finish (abfd, &preserve_match);
       bfd_preserve_finish (abfd, &preserve);
-      bfd_set_error_handler (orig_error_handler);
+      _bfd_restore_error_handler_caching (orig_messages);
 
-      struct per_xvec_message **list = _bfd_per_xvec_warn (abfd->xvec, 0);
-      if (*list)
-	print_warnmsg (list);
-      list = _bfd_per_xvec_warn (NULL, 0);
-      for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
-	clear_warnmsg (list++);
-      --in_check_format;
+      print_and_clear_messages (&messages, abfd->xvec);
 
       /* File position has moved, BTW.  */
       return true;
@@ -643,27 +656,8 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   if (preserve_match.marker != NULL)
     bfd_preserve_finish (abfd, &preserve_match);
   bfd_preserve_restore (abfd, &preserve);
-  bfd_set_error_handler (orig_error_handler);
-  struct per_xvec_message **list = _bfd_per_xvec_warn (NULL, 0);
-  struct per_xvec_message **one = NULL;
-  for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
-    {
-      if (list[i])
-	{
-	  if (!one)
-	    one = list + i;
-	  else
-	    {
-	      one = NULL;
-	      break;
-	    }
-	}
-    }
-  if (one)
-    print_warnmsg (one);
-  for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
-    clear_warnmsg (list++);
-  --in_check_format;
+  _bfd_restore_error_handler_caching (orig_messages);
+  print_and_clear_messages (&messages, NULL);
   return false;
 }
 
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index f15b5f27db8..04dbe720edb 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -934,7 +934,9 @@ void _bfd_clear_error_data (void) ATTRIBUTE_HIDDEN;
 
 char *bfd_asprintf (const char *fmt, ...) ATTRIBUTE_HIDDEN;
 
-bfd_error_handler_type _bfd_set_error_handler_caching (bfd *) ATTRIBUTE_HIDDEN;
+struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *) ATTRIBUTE_HIDDEN;
+
+void _bfd_restore_error_handler_caching (struct per_xvec_messages *) ATTRIBUTE_HIDDEN;
 
 const char *_bfd_get_error_program_name (void) ATTRIBUTE_HIDDEN;
 
@@ -3699,7 +3701,18 @@ struct per_xvec_message
   char message[];
 };
 
-struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t) ATTRIBUTE_HIDDEN;
+/* A list of per_xvec_message objects.  The targ field
+   indicates which xvec this list holds.  The abfd field is
+   only needed in the root entry of the list.  */
+struct per_xvec_messages
+{
+  bfd *abfd;
+  const bfd_target *targ;
+  struct per_xvec_message *messages;
+  struct per_xvec_messages *next;
+};
+
+struct per_xvec_message *_bfd_per_xvec_warn (struct per_xvec_messages *, size_t) ATTRIBUTE_HIDDEN;
 
 #ifdef __cplusplus
 }
diff --git a/bfd/targets.c b/bfd/targets.c
index f94992d031d..baf3cc5266c 100644
--- a/bfd/targets.c
+++ b/bfd/targets.c
@@ -1457,9 +1457,6 @@ const bfd_target *const *const bfd_associated_vector = _bfd_associated_vector;
    number of entries that the array could possibly need.  */
 const size_t _bfd_target_vector_entries = ARRAY_SIZE (_bfd_target_vector);
 
-/* A place to stash a warning from _bfd_check_format.  */
-static struct per_xvec_message *per_xvec_warn[ARRAY_SIZE (_bfd_target_vector)
-					      + 1];
 \f
 /* This array maps configuration triplets onto BFD vectors.  */
 
@@ -1488,11 +1485,22 @@ INTERNAL
 .  char message[];
 .};
 .
+.{* A list of per_xvec_message objects.  The targ field
+.   indicates which xvec this list holds.  The abfd field is
+.   only needed in the root entry of the list.  *}
+.struct per_xvec_messages
+.{
+.  bfd *abfd;
+.  const bfd_target *targ;
+.  struct per_xvec_message *messages;
+.  struct per_xvec_messages *next;
+.};
+.
 INTERNAL_FUNCTION
 	_bfd_per_xvec_warn
 
 SYNOPSIS
-	struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t);
+	struct per_xvec_message *_bfd_per_xvec_warn (struct per_xvec_messages *, size_t);
 
 DESCRIPTION
 	Return a location for the given target xvec to use for
@@ -1505,19 +1513,36 @@ DESCRIPTION
 	pointer to a NULL pointer on allocation failure.
 */
 
-struct per_xvec_message **
-_bfd_per_xvec_warn (const bfd_target *targ, size_t alloc)
+struct per_xvec_message *
+_bfd_per_xvec_warn (struct per_xvec_messages *messages, size_t alloc)
 {
-  size_t idx;
-
-  if (!targ)
-    return per_xvec_warn;
-  for (idx = 0; idx < ARRAY_SIZE (_bfd_target_vector); idx++)
-    if (_bfd_target_vector[idx] == targ)
-      break;
-  struct per_xvec_message **m = per_xvec_warn + idx;
-  if (!alloc)
-    return m;
+  const bfd_target *targ = messages->abfd->xvec;
+
+  struct per_xvec_messages *prev = NULL;
+  struct per_xvec_messages *iter = messages;
+
+  for (iter = messages; iter != NULL; iter = iter->next)
+    {
+      if (iter->targ == targ || iter->targ == NULL)
+	break;
+      prev = iter;
+    }
+
+  if (iter == NULL)
+    {
+      iter = bfd_malloc (sizeof (*iter));
+      if (iter == NULL)
+	return NULL;
+      iter->abfd = messages->abfd;
+      iter->targ = targ;
+      iter->messages = NULL;
+      iter->next = NULL;
+      prev->next = iter;
+    }
+  else if (iter->targ == NULL)
+    iter->targ = targ;
+
+  struct per_xvec_message **m = &iter->messages;
   int count = 0;
   while (*m)
     {
@@ -1531,7 +1556,7 @@ _bfd_per_xvec_warn (const bfd_target *targ, size_t alloc)
       if (*m != NULL)
 	(*m)->next = NULL;
     }
-  return m;
+  return *m;
 }
 
 /* Find a target vector, given a name or configuration triplet.  */
-- 
2.43.0