[1/3] Make several more BFD globals thread-local

  Among other things, PR gdb/31264 points out a race in bfd_check_format
-- it sets the error handler, which is a global.

Looking into this a bit more, I found several other possible races:
the "in_check_format" static local variable in
bfd_check_format_matches, and the contents of per_xvec_warn.

This patch makes all of these thread-local.

I don't actually think this is the best way to approach this.
"in_check_format" and the per-xvec warnings could be done by setting a
flag on the BFD, avoiding globals entirely.  I can do that if you
want; I just wasn't sure if that is desirable or not.

Note that this bug also points out a race between bfd_cache_close_all
and bfd_check_format; I am not sure yet how this should be handled.

bfd/ChangeLog
2024-01-29  Tom Tromey  <tom@tromey.com>

	PR gdb/31264
	* targets.c (per_xvec_warn): Now thread-local.
	* format.c (bfd_check_format_matches): Mark "in_check_format" as
	thread-local.
	* bfd.c (_bfd_error_internal, error_handler_bfd): Now
	thread-local.
---
 bfd/ChangeLog | 9 +++++++++
 bfd/bfd.c     | 4 ++--
 bfd/format.c  | 2 +-
 bfd/targets.c | 4 ++--
 4 files changed, 14 insertions(+), 5 deletions(-)
  

Hi Tom,

> Among other things, PR gdb/31264 points out a race in bfd_check_format
> -- it sets the error handler, which is a global.
> 
> Looking into this a bit more, I found several other possible races:
> the "in_check_format" static local variable in
> bfd_check_format_matches, and the contents of per_xvec_warn.
> 
> This patch makes all of these thread-local.
> 
> I don't actually think this is the best way to approach this.
> "in_check_format" and the per-xvec warnings could be done by setting a
> flag on the BFD, avoiding globals entirely.  I can do that if you
> want; I just wasn't sure if that is desirable or not.

Actually, I think that I would prefer this alternative solution.

I am particularly worried about making the error handler thread local
when this might not match the expectations of a client using the BFD
library.  Since the error handler is currently global, a client can
expect to set it once and have it affect all threads.

Looking at the code in bfd_check_format_matches() it seems clear that
the error handler manipulation is really a hack, based upon the single
threaded nature of the BFD library.  This become problematical when the
BFD library is used in a multi-threaded environment.

It seems to me that the proper thing to do would be to [throw away the
BFD library and use something designed from the ground up to thread-safe],
ahem, I mean provide a thread safe way for bfd_check_format_matches to
override the error handler within its local context, without it affecting
the global error handler for other threads *and* still allowing the client
to call bfd_set_error_handler to change the error handler for all threads
at once.

This sounds like a lot of work however, and probably something that
ought to be done as part of a larger project to turn the BFD library
into a thread-safe and multi-threaded library.

What are you thoughts ?

Cheers
   Nick
  

>>>>> "Nick" == Nick Clifton <nickc@redhat.com> writes:

Nick> I am particularly worried about making the error handler thread local
Nick> when this might not match the expectations of a client using the BFD
Nick> library.  Since the error handler is currently global, a client can
Nick> expect to set it once and have it affect all threads.

I suppose so, but it's perhaps worth pointing out that thread-safety is
new, and AFAIK only gdb uses it.  So at least IMO there's a bit of
leeway to define how this should work in the future, rather than feeling
bound by what happens to happen today.

[...]
Nick> This sounds like a lot of work however, and probably something that
Nick> ought to be done as part of a larger project to turn the BFD library
Nick> into a thread-safe and multi-threaded library.

Nick> What are you thoughts ?

I will refresh my knowledge and get back to you soon.

Meanwhile I'm going to push the two patches you approved, since they are
actually somewhat independent of this particular issue.

thanks
Tom
  

>> Among other things, PR gdb/31264 points out a race in bfd_check_format
>> -- it sets the error handler, which is a global.
>> Looking into this a bit more, I found several other possible races:
>> the "in_check_format" static local variable in
>> bfd_check_format_matches, and the contents of per_xvec_warn.
>> This patch makes all of these thread-local.
>> I don't actually think this is the best way to approach this.
>> "in_check_format" and the per-xvec warnings could be done by setting a
>> flag on the BFD, avoiding globals entirely.  I can do that if you
>> want; I just wasn't sure if that is desirable or not.

Nick> Actually, I think that I would prefer this alternative solution.

Nick> I am particularly worried about making the error handler thread local
[...]

Ok, I looked back at the code again.

What I meant when I wrote the above is that basically there are two sets
of globals to consider in this patch: the ones related to error emission
(_bfd_error_internal and error_handler_bfd), and then the other ones
related to the caching error message code (per_xvec_warn,
in_check_format).

I thought that making the latter ones thread-local felt hackish, because
they could somewhat easily be attributes of a BFD and not globals at
all.

_bfd_error_internal is a lot harder to change.  It could be done in
theory -- the idea would be to pass a BFD to every call to
_bfd_error_handler.  However, there are ~1400 such calls in BFD, which
is a quite a lot, and then there's also the unfortunate:

#define opcodes_error_handler _bfd_error_handler

... where, I assume, many calls don't even have access to a BFD.

Now, to address your concern about _bfd_error_internal (i.e., not making
it thread-local), I think we can add another thread-local flag, just for
bfd_check_format_matches.  The idea here would be that
_bfd_error_handler would check this flag, and then
_bfd_set_error_handler_caching would simply set this flag -- and not
change the BFD error handler at all.

I think this approach would be invisible to BFD clients.

Nick> This sounds like a lot of work however, and probably something that
Nick> ought to be done as part of a larger project to turn the BFD library
Nick> into a thread-safe and multi-threaded library.

FWIW I'd like to say that BFD really isn't too bad on this front.  The
main thing BFD has going for it is that nearly everything (and
especially in the reading side) is done via different BFD objects, so
there's a kind of natural lack of globals.

The remaining big issue here is interacting with the fd cache code,
which we discussed a little bit a while ago (last year?).  Here I think
the problem is that bfd_check_format_matches can change the BFD's iovec,
which interacts weirdly with cache.c; and as the cache involves globals
it means that the lock has to be held somewhere.

Tom
  

Tom> What I meant when I wrote the above is that basically there are two sets
Tom> of globals to consider in this patch: the ones related to error emission
Tom> (_bfd_error_internal and error_handler_bfd), and then the other ones
Tom> related to the caching error message code (per_xvec_warn,
Tom> in_check_format).

Tom> I thought that making the latter ones thread-local felt hackish, because
Tom> they could somewhat easily be attributes of a BFD and not globals at
Tom> all.

I took a stab at some of this tonight and discovered that since
in_check_format is only used by the error-reporting code, it can be
combined with the other flag.

I've appended a rough draft of this.

I haven't tackled the xvec stuff yet.

Also I think it'd be good if print_warnmsg re-emitted the errors using
_bfd_error_handler.  This would let gdb print them the way it likes.
However... that's a change to semantics so I don't know if that's OK or
whether another error emitter is required.  I guess I don't really know
how to provoke one of these messages and when they would matter.  Like,
I see that ldmain.c installs an error handler -- is it intentional that
this be bypassed by errors occurring in bfd_check_format_matches?

thanks,
Tom

diff --git a/bfd/bfd.c b/bfd/bfd.c
index 6c822656cc8..238aa71b4c5 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1528,9 +1528,15 @@ err_sprintf (void *stream, const char *fmt, ...)
 }
 
 /* Communicate the bfd processed by bfd_check_format_matches to the
-   error handling function error_handler_sprintf.  */
+   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 bfd *error_handler_bfd;
+
+/* A special value for error_handler_bfd that indicates that the error
+   should simply be ignored.  */
+#define IGNORE_ERROR_BFD ((bfd *) -1)
 
 /* An error handler that prints to a string, then dups that string to
    a per-xvec cache.  */
@@ -1590,7 +1596,14 @@ _bfd_error_handler (const char *fmt, ...)
   va_list ap;
 
   va_start (ap, fmt);
-  _bfd_error_internal (fmt, ap);
+  if (error_handler_bfd == IGNORE_ERROR_BFD)
+    {
+      /* Nothing.  */
+    }
+  else if (error_handler_bfd != NULL)
+    error_handler_sprintf (fmt, ap);
+  else
+    _bfd_error_internal (fmt, ap);
   va_end (ap);
 }
 
@@ -1621,18 +1634,42 @@ INTERNAL_FUNCTION
 	_bfd_set_error_handler_caching
 
 SYNOPSIS
-	bfd_error_handler_type _bfd_set_error_handler_caching (bfd *);
+	bfd *_bfd_set_error_handler_caching (bfd *);
 
 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 *
 _bfd_set_error_handler_caching (bfd *abfd)
+{
+  bfd *old = error_handler_bfd;
+  if (old == NULL)
+    error_handler_bfd = abfd;
+  else
+    error_handler_bfd = IGNORE_ERROR_BFD;
+  return old;
+}
+
+/*
+INTERNAL_FUNCTION
+	_bfd_restore_error_handler_caching
+
+SYNOPSIS
+	void _bfd_restore_error_handler_caching (bfd *);
+
+DESCRIPTION
+	Reset the BFD error handler function an earlier value.
+*/
+
+void
+_bfd_restore_error_handler_caching (bfd *abfd)
 {
   error_handler_bfd = abfd;
-  return bfd_set_error_handler (error_handler_sprintf);
 }
 
 /*
diff --git a/bfd/format.c b/bfd/format.c
index 47c3e9ba35a..42b50421288 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -279,12 +279,6 @@ clear_warnmsg (struct per_xvec_message **list)
   *list = NULL;
 }
 
-static void
-null_error_handler (const char *fmt ATTRIBUTE_UNUSED,
-		    va_list ap ATTRIBUTE_UNUSED)
-{
-}
-
 /*
 FUNCTION
 	bfd_check_format_matches
@@ -320,8 +314,7 @@ 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;
+  bfd *orig_error_bfd;
 
   if (matching != NULL)
     *matching = NULL;
@@ -350,13 +343,10 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   abfd->format = format;
   save_targ = abfd->xvec;
 
-  /* 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;
+  /* We don't want to report errors on recursive calls checking the
+     first element of an archive.  This is handled by the
+     error-handler code, which see.  */
+  orig_error_bfd = _bfd_set_error_handler_caching (abfd);
 
   preserve_match.marker = NULL;
   if (!bfd_preserve_save (abfd, &preserve, NULL))
@@ -598,7 +588,7 @@ 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_error_bfd);
 
       struct per_xvec_message **list = _bfd_per_xvec_warn (abfd->xvec, 0);
       if (*list)
@@ -606,7 +596,6 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
       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;
 
       /* File position has moved, BTW.  */
       return true;
@@ -650,7 +639,7 @@ 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);
+  _bfd_restore_error_handler_caching (orig_error_bfd);
   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++)
@@ -670,7 +659,6 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
     print_warnmsg (one);
   for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
     clear_warnmsg (list++);
-  --in_check_format;
   return false;
 }
 
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index 40bbe6a3886..40e9c88e711 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -933,7 +933,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;
+bfd *_bfd_set_error_handler_caching (bfd *) ATTRIBUTE_HIDDEN;
+
+void _bfd_restore_error_handler_caching (bfd *) ATTRIBUTE_HIDDEN;
 
 const char *_bfd_get_error_program_name (void) ATTRIBUTE_HIDDEN;