[PATCH 0/3] Fix some error-printing issues

[PATCH 0/3] Fix some error-printing issues

[Tom Tromey]

This series came out of a race reported to gdb bugzilla.  This is
fixed in patch 1, though perhaps a different approach would be better
for some of it -- I'd appreciate comments on this, I don't mind
redoing it.

Also there's another race in here that I haven't tackled yet, where
bfd_check_format can race with bfd_cache_close_all.

The next two patches clean up the error reporting system a little, to
make it friendlier to gdb.

Let me know what you think.

Tom

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

[Tom Tromey]

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(-)

diff --git a/bfd/bfd.c b/bfd/bfd.c
index 0776145af52..5619799e403 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1504,7 +1504,7 @@ err_sprintf (void *stream, const char *fmt, ...)
 /* Communicate the bfd processed by bfd_check_format_matches to the
    error handling function error_handler_sprintf.  */
 
-static bfd *error_handler_bfd;
+static TLS bfd *error_handler_bfd;
 
 /* An error handler that prints to a string, then dups that string to
    a per-xvec cache.  */
@@ -1538,7 +1538,7 @@ error_handler_sprintf (const char *fmt, va_list ap)
    function pointer permits a program linked against BFD to intercept
    the messages and deal with them itself.  */
 
-static bfd_error_handler_type _bfd_error_internal = error_handler_fprintf;
+static TLS bfd_error_handler_type _bfd_error_internal = error_handler_fprintf;
 
 /*
 FUNCTION
diff --git a/bfd/format.c b/bfd/format.c
index 47c3e9ba35a..ec37441904c 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -321,7 +321,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   struct bfd_preserve preserve, preserve_match;
   bfd_cleanup cleanup = NULL;
   bfd_error_handler_type orig_error_handler;
-  static int in_check_format;
+  static TLS int in_check_format;
 
   if (matching != NULL)
     *matching = NULL;
diff --git a/bfd/targets.c b/bfd/targets.c
index 3addf2fe373..ca2a2039217 100644
--- a/bfd/targets.c
+++ b/bfd/targets.c
@@ -1458,8 +1458,8 @@ const bfd_target *const *const bfd_associated_vector = _bfd_associated_vector;
 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];
+static TLS struct per_xvec_message *per_xvec_warn[ARRAY_SIZE (_bfd_target_vector)
+						  + 1];
 \f
 /* This array maps configuration triplets onto BFD vectors.  */
 
-- 
2.43.0

[PATCH 2/3] Do not call fputc from _bfd_doprnt

[Tom Tromey]

I noticed that _bfd_doprnt can unconditionally call fputc.  However,
when called from error_handler_sprintf, this will likely result in a
crash, as the stream argument does not actually point to a FILE.

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

	* bfd.c (_bfd_doprnt): Do not call fputc.
---
 bfd/ChangeLog | 4 ++++
 bfd/bfd.c     | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/bfd/bfd.c b/bfd/bfd.c
index 5619799e403..0f1eaa1629f 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1027,7 +1027,7 @@ _bfd_doprnt (print_func print, void *stream, const char *format,
 	}
       else if (ptr[1] == '%')
 	{
-	  fputc ('%', stream);
+	  print (stream, "%%");
 	  result = 1;
 	  ptr += 2;
 	}
-- 
2.43.0

[PATCH 3/3] Introduce bfd_print_error function

[Tom Tromey]

gdb likes to control its own output; for example, this is important
for gdb's pager, and for logging.  While BFD provides a way to
intercept error output, via bfd_set_error_handler, it turns out to be
difficult for this function to truly generate the desired output in a
gdb-friendly way -- the error handler is expected to implement some
BFD printf format extensions.

This patch introduces a new function that an error handler can use to
format the text.  This way, gdb can set the error handler and arrange
for the output to be displayed as it likes.

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

	* bfd.c (bfd_print_callback): Rename from print_func.  Move into
	comment.
	(_bfd_doprnt): Update.
	(bfd_print_error): New function.
	(error_handler_fprintf, error_handler_sprintf): Use
	bfd_print_error.
	* bfd-in2.h: Rebuild.
---
 bfd/ChangeLog | 10 ++++++++++
 bfd/bfd-in2.h |  4 ++++
 bfd/bfd.c     | 48 ++++++++++++++++++++++++++++++++++++------------
 3 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 581d8fe0b3e..e8b0e9f8699 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -2557,6 +2557,10 @@ void bfd_perror (const char *message);
 
 typedef void (*bfd_error_handler_type) (const char *, va_list);
 
+typedef int (*bfd_print_callback) (void *, const char *, ...);
+void bfd_print_error (bfd_print_callback print_func,
+    void *stream, const char *fmt, va_list ap);
+
 void _bfd_error_handler (const char *fmt, ...) ATTRIBUTE_PRINTF_1;
 
 bfd_error_handler_type bfd_set_error_handler (bfd_error_handler_type);
diff --git a/bfd/bfd.c b/bfd/bfd.c
index 0f1eaa1629f..886a97ce078 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1000,10 +1000,13 @@ union _bfd_doprnt_args
       result = print (stream, specifier, value);		\
     } while (0)
 
-typedef int (*print_func) (void *, const char *, ...);
+/*
+CODE_FRAGMENT
+.typedef int (*bfd_print_callback) (void *, const char *, ...);
+*/
 
 static int
-_bfd_doprnt (print_func print, void *stream, const char *format,
+_bfd_doprnt (bfd_print_callback print, void *stream, const char *format,
 	     union _bfd_doprnt_args *args)
 {
   const char *ptr = format;
@@ -1446,21 +1449,44 @@ _bfd_doprnt_scan (const char *format, va_list ap, union _bfd_doprnt_args *args)
   return arg_count;
 }
 
-/* The standard error handler that prints to stderr.  */
+/*
+FUNCTION
+	bfd_print_error
 
-static void
-error_handler_fprintf (const char *fmt, va_list ap)
+SYNOPSIS
+	void bfd_print_error (bfd_print_callback print_func,
+	  void *stream, const char *fmt, va_list ap);
+
+DESCRIPTION
+
+	This formats FMT and AP according to BFD "printf" rules,
+	sending the output to STREAM by repeated calls to PRINT_FUNC.
+	PRINT_FUNC is a printf-like function; it does not need to
+	implement the BFD printf format extensions.  This can be used
+	in a callback that is set via bfd_set_error_handler to turn
+	the error into ordinary output.
+*/
+
+void
+bfd_print_error (bfd_print_callback print_func, void *stream,
+		 const char *fmt, va_list ap)
 {
   union _bfd_doprnt_args args[MAX_ARGS];
 
+  print_func (stream, "%s: ", _bfd_get_error_program_name ());
   _bfd_doprnt_scan (fmt, ap, args);
+  _bfd_doprnt (print_func, stream, fmt, args);
+}
 
+/* The standard error handler that prints to stderr.  */
+
+static void
+error_handler_fprintf (const char *fmt, va_list ap)
+{
   /* PR 4992: Don't interrupt output being sent to stdout.  */
   fflush (stdout);
 
-  fprintf (stderr, "%s: ", _bfd_get_error_program_name ());
-
-  _bfd_doprnt ((print_func) fprintf, stderr, fmt, args);
+  bfd_print_error ((bfd_print_callback) fprintf, stderr, fmt, ap);
 
   /* On AIX, putc is implemented as a macro that triggers a -Wunused-value
      warning, so use the fputc function to avoid it.  */
@@ -1512,15 +1538,13 @@ static TLS bfd *error_handler_bfd;
 static void
 error_handler_sprintf (const char *fmt, va_list ap)
 {
-  union _bfd_doprnt_args args[MAX_ARGS];
   char error_buf[1024];
   struct buf_stream error_stream;
 
-  _bfd_doprnt_scan (fmt, ap, args);
-
   error_stream.ptr = error_buf;
   error_stream.left = sizeof (error_buf);
-  _bfd_doprnt (err_sprintf, &error_stream, fmt, args);
+
+  bfd_print_error (err_sprintf, &error_stream, fmt, ap);
 
   size_t len = error_stream.ptr - error_buf;
   struct per_xvec_message **warn
-- 
2.43.0

Re: [PATCH 2/3] Do not call fputc from _bfd_doprnt

[Nick Clifton]

Hi Tom,

> I noticed that _bfd_doprnt can unconditionally call fputc.  However,
> when called from error_handler_sprintf, this will likely result in a
> crash, as the stream argument does not actually point to a FILE.
> 
> bfd/ChangeLog
> 2024-01-29  Tom Tromey  <tom@tromey.com>
> 
> 	* bfd.c (_bfd_doprnt): Do not call fputc.

Approved - please apply.

Cheers
   Nick

Re: [PATCH 3/3] Introduce bfd_print_error function

[Nick Clifton]

Hi Tom,

> gdb likes to control its own output; for example, this is important
> for gdb's pager, and for logging.  While BFD provides a way to
> intercept error output, via bfd_set_error_handler, it turns out to be
> difficult for this function to truly generate the desired output in a
> gdb-friendly way -- the error handler is expected to implement some
> BFD printf format extensions.
> 
> This patch introduces a new function that an error handler can use to
> format the text.  This way, gdb can set the error handler and arrange
> for the output to be displayed as it likes.
> 
> bfd/ChangeLog
> 2024-01-29  Tom Tromey  <tom@tromey.com>
> 
> 	* bfd.c (bfd_print_callback): Rename from print_func.  Move into
> 	comment.
> 	(_bfd_doprnt): Update.
> 	(bfd_print_error): New function.
> 	(error_handler_fprintf, error_handler_sprintf): Use
> 	bfd_print_error.
> 	* bfd-in2.h: Rebuild.

Approved - please apply.

Cheers
   Nick

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

[Nick Clifton]

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

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

[Tom Tromey]

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

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

[Tom Tromey]

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

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

[Tom Tromey]

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;
 

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

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

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.

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

Tom> I've appended a rough draft of this.

Tom> I haven't tackled the xvec stuff yet.

I looked into this problem this evening and I found out that the
per-xvec errors are also only used from inside bfd_check_format_matches.
A (thread-local) global would still be needed, because warnings aren't
emitted "per-BFD" -- but it could just replace error_handler_bfd,
leaving just a single global.

I haven't implemented this but I plan to try it soon.  This is one of
the issues blocking GDB 15.

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

I'd appreciate some insight into this.

thanks,
Tom

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

[Alan Modra]

On Fri, Mar 08, 2024 at 06:12:34PM -0700, Tom Tromey wrote:
> >>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> Tom> Also I think it'd be good if print_warnmsg re-emitted the errors using
> Tom> _bfd_error_handler.  This would let gdb print them the way it likes.
> Tom> However... that's a change to semantics so I don't know if that's OK or
> Tom> whether another error emitter is required.  I guess I don't really know
> Tom> how to provoke one of these messages and when they would matter.

See 5aa0f10c424e commit log.
The testcase in https://oss-fuzz.com/testcase-detail/4772285297328128
is the one that prompted all the messing around with errors.  Try
objdump or nm from 2.36 on that one..

> I'd appreciate some insight into this.

Yes, print_warnmsg should indeed be using _bfd_error_handler.  Both
calls to print_warnmsg are preceeded by setting the error handler back
to the original one.

This also fixes a duplicate program name output.

	* bfd.c (bfd_print_error): Make static.  Don't print program name.
	(error_handler_fprintf): Print program name here.
	* format.c (print_warnmsg): Use _bfd_error_handler to print
	cached messages.
	* bfd-in2.h: Regenerate.

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 79b24a7f6e9..76d450478a7 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -2558,9 +2558,6 @@ void bfd_perror (const char *message);
 typedef void (*bfd_error_handler_type) (const char *, va_list);
 
 typedef int (*bfd_print_callback) (void *, const char *, ...);
-void bfd_print_error (bfd_print_callback print_func,
-    void *stream, const char *fmt, va_list ap);
-
 void _bfd_error_handler (const char *fmt, ...) ATTRIBUTE_PRINTF_1;
 
 bfd_error_handler_type bfd_set_error_handler (bfd_error_handler_type);
diff --git a/bfd/bfd.c b/bfd/bfd.c
index 71732a0f92b..1bdfac6c24c 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1449,31 +1449,12 @@ _bfd_doprnt_scan (const char *format, va_list ap, union _bfd_doprnt_args *args)
   return arg_count;
 }
 
-/*
-FUNCTION
-	bfd_print_error
-
-SYNOPSIS
-	void bfd_print_error (bfd_print_callback print_func,
-	  void *stream, const char *fmt, va_list ap);
-
-DESCRIPTION
-
-	This formats FMT and AP according to BFD "printf" rules,
-	sending the output to STREAM by repeated calls to PRINT_FUNC.
-	PRINT_FUNC is a printf-like function; it does not need to
-	implement the BFD printf format extensions.  This can be used
-	in a callback that is set via bfd_set_error_handler to turn
-	the error into ordinary output.
-*/
-
-void
+static void
 bfd_print_error (bfd_print_callback print_func, void *stream,
 		 const char *fmt, va_list ap)
 {
   union _bfd_doprnt_args args[MAX_ARGS];
 
-  print_func (stream, "%s: ", _bfd_get_error_program_name ());
   _bfd_doprnt_scan (fmt, ap, args);
   _bfd_doprnt (print_func, stream, fmt, args);
 }
@@ -1486,6 +1467,7 @@ error_handler_fprintf (const char *fmt, va_list ap)
   /* PR 4992: Don't interrupt output being sent to stdout.  */
   fflush (stdout);
 
+  fprintf (stderr, "%s: ", _bfd_get_error_program_name ());
   bfd_print_error ((bfd_print_callback) fprintf, stderr, fmt, ap);
 
   /* On AIX, putc is implemented as a macro that triggers a -Wunused-value
diff --git a/bfd/format.c b/bfd/format.c
index 47c3e9ba35a..8f3fc7e7b96 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -255,15 +255,8 @@ bfd_preserve_finish (bfd *abfd ATTRIBUTE_UNUSED, struct bfd_preserve *preserve)
 static void
 print_warnmsg (struct per_xvec_message **list)
 {
-  fflush (stdout);
-  fprintf (stderr, "%s: ", _bfd_get_error_program_name ());
-
   for (struct per_xvec_message *warn = *list; warn; warn = warn->next)
-    {
-      fputs (warn->message, stderr);
-      fputc ('\n', stderr);
-    }
-  fflush (stderr);
+    _bfd_error_handler ("%s", warn->message);
 }
 
 static void


-- 
Alan Modra
Australia Development Lab, IBM

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

[Alan Modra]

On Sat, Mar 09, 2024 at 02:41:13PM +1030, Alan Modra wrote:
> This also fixes a duplicate program name output.
> 
> 	* bfd.c (bfd_print_error): Make static.  Don't print program name.

Oops, sorry.  I didn't see any uses of bfd_print_error in the sources
and didn't check its history.

	* bfd.c (_bfd_print): Renamed from bfd_print_error.
	(bfd_print_error): Reinstate previous code but using the above.
	(error_handler_fprintf, error_handler_sprintf): Adjust.
	* bfd-in2.h: Regenerate.

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 76d450478a7..79b24a7f6e9 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -2558,6 +2558,9 @@ void bfd_perror (const char *message);
 typedef void (*bfd_error_handler_type) (const char *, va_list);
 
 typedef int (*bfd_print_callback) (void *, const char *, ...);
+void bfd_print_error (bfd_print_callback print_func,
+    void *stream, const char *fmt, va_list ap);
+
 void _bfd_error_handler (const char *fmt, ...) ATTRIBUTE_PRINTF_1;
 
 bfd_error_handler_type bfd_set_error_handler (bfd_error_handler_type);
diff --git a/bfd/bfd.c b/bfd/bfd.c
index f40cf941edc..54061a34240 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1450,8 +1450,8 @@ _bfd_doprnt_scan (const char *format, va_list ap, union _bfd_doprnt_args *args)
 }
 
 static void
-bfd_print_error (bfd_print_callback print_func, void *stream,
-		 const char *fmt, va_list ap)
+_bfd_print (bfd_print_callback print_func, void *stream,
+	    const char *fmt, va_list ap)
 {
   union _bfd_doprnt_args args[MAX_ARGS];
 
@@ -1459,6 +1459,32 @@ bfd_print_error (bfd_print_callback print_func, void *stream,
   _bfd_doprnt (print_func, stream, fmt, args);
 }
 
+/*
+FUNCTION
+	bfd_print_error
+
+SYNOPSIS
+	void bfd_print_error (bfd_print_callback print_func,
+	  void *stream, const char *fmt, va_list ap);
+
+DESCRIPTION
+
+	This formats FMT and AP according to BFD "printf" rules,
+	sending the output to STREAM by repeated calls to PRINT_FUNC.
+	PRINT_FUNC is a printf-like function; it does not need to
+	implement the BFD printf format extensions.  This can be used
+	in a callback that is set via bfd_set_error_handler to turn
+	the error into ordinary output.
+*/
+
+void
+bfd_print_error (bfd_print_callback print_func, void *stream,
+		 const char *fmt, va_list ap)
+{
+  print_func (stream, "%s: ", _bfd_get_error_program_name ());
+  _bfd_print (print_func, stream, fmt, ap);
+}
+
 /* The standard error handler that prints to stderr.  */
 
 static void
@@ -1467,7 +1493,6 @@ error_handler_fprintf (const char *fmt, va_list ap)
   /* PR 4992: Don't interrupt output being sent to stdout.  */
   fflush (stdout);
 
-  fprintf (stderr, "%s: ", _bfd_get_error_program_name ());
   bfd_print_error ((bfd_print_callback) fprintf, stderr, fmt, ap);
 
   /* On AIX, putc is implemented as a macro that triggers a -Wunused-value
@@ -1526,7 +1551,7 @@ error_handler_sprintf (const char *fmt, va_list ap)
   error_stream.ptr = error_buf;
   error_stream.left = sizeof (error_buf);
 
-  bfd_print_error (err_sprintf, &error_stream, fmt, ap);
+  _bfd_print (err_sprintf, &error_stream, fmt, ap);
 
   size_t len = error_stream.ptr - error_buf;
   struct per_xvec_message **warn

-- 
Alan Modra
Australia Development Lab, IBM

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

[Tom Tromey]

>>>>> "Alan" == Alan Modra <amodra@gmail.com> writes:

Alan> On Sat, Mar 09, 2024 at 02:41:13PM +1030, Alan Modra wrote:
>> This also fixes a duplicate program name output.
>> 
>> * bfd.c (bfd_print_error): Make static.  Don't print program name.

Alan> Oops, sorry.  I didn't see any uses of bfd_print_error in the sources
Alan> and didn't check its history.

Thanks, I appreciate the update.  I sent the rewrite of gdb's error
handler to gdb-patches today.

I also have a patch to handle thread-safety with the per-xvec messages.
I'm going test that with the bug you mentioned before submitting it.

Tom

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

[Tom Tromey]

>>>>> "Alan" == Alan Modra <amodra@gmail.com> writes:

Alan> On Fri, Mar 08, 2024 at 06:12:34PM -0700, Tom Tromey wrote:
>> >>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> Also I think it'd be good if print_warnmsg re-emitted the errors using
Tom> _bfd_error_handler.  This would let gdb print them the way it likes.
Tom> However... that's a change to semantics so I don't know if that's OK or
Tom> whether another error emitter is required.  I guess I don't really know
Tom> how to provoke one of these messages and when they would matter.

Alan> See 5aa0f10c424e commit log.
Alan> The testcase in https://oss-fuzz.com/testcase-detail/4772285297328128
Alan> is the one that prompted all the messing around with errors.  Try
Alan> objdump or nm from 2.36 on that one..

Unfortunately I couldn't access it.  Is there any chance you could send
it to me so I can try it against my patch?

thanks,
Tom

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

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Alan> Oops, sorry.  I didn't see any uses of bfd_print_error in the sources
Alan> and didn't check its history.

Tom> Thanks, I appreciate the update.  I sent the rewrite of gdb's error
Tom> handler to gdb-patches today.

Tom> I also have a patch to handle thread-safety with the per-xvec messages.
Tom> I'm going test that with the bug you mentioned before submitting it.

Quick update on this...

I sent another note about my inability to access the test.

While looking into this, though, I couldn't reproduce the exact race
mentioned in the bug.  However, I still believe it is there.  And, my
testing shows another race, this time between a call to
bfd_cache_close_all on one thread, and bfd_check_format_matches on
another.  I plan to rework my patch to account for this as well, I think
by having bfd_cache_close_all skip any BFDs currently being
format-matched.

Tom