[PATCH] Rewrite gdb_bfd_error_handler

[PATCH] Rewrite gdb_bfd_error_handler

[Tom Tromey]

This patch rewrites gdb_bfd_error_handler to use 'bfd_print_error' to
generate the text of the warning, and then emits it using 'warning'.
The current code in the tree is a bit wrong because it may do the
wrong thing when BFD uses ones of its printf extensions.

This also adds locking to increment_bfd_error_count.  This is
important now that some BFD operations can be done on worker threads.

This approach makes it simpler for worker threads to intercept any
messages.

Regression tested on x86-64 Fedora 38.
---
 gdb/gdb_bfd.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 4df1e66a388..b579ba0cc92 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -1180,15 +1180,30 @@ get_bfd_inferior_data (struct inferior *inf)
    count.  */
 
 static unsigned long
-increment_bfd_error_count (std::string str)
+increment_bfd_error_count (const std::string &str)
 {
+#if CXX_STD_THREAD
+  std::lock_guard<std::recursive_mutex> guard (gdb_bfd_mutex);
+#endif
   struct bfd_inferior_data *bid = get_bfd_inferior_data (current_inferior ());
 
   auto &map = bid->bfd_error_string_counts;
-  return ++map[std::move (str)];
+  return ++map[str];
 }
 
-static bfd_error_handler_type default_bfd_error_handler;
+/* A print callback for bfd_print_error.  */
+
+static int
+print_error_callback (void *stream, const char *fmt, ...)
+{
+  string_file *file = (string_file *) stream;
+  size_t in_size = file->size ();
+  va_list ap;
+  va_start (ap, fmt);
+  file->vprintf (fmt, ap);
+  va_end (ap);
+  return file->size () - in_size;
+}
 
 /* Define a BFD error handler which will suppress the printing of
    messages which have been printed once already.  This is done on a
@@ -1197,20 +1212,14 @@ static bfd_error_handler_type default_bfd_error_handler;
 static void ATTRIBUTE_PRINTF (1, 0)
 gdb_bfd_error_handler (const char *fmt, va_list ap)
 {
-  va_list ap_copy;
-
-  va_copy(ap_copy, ap);
-  const std::string str = string_vprintf (fmt, ap_copy);
-  va_end (ap_copy);
+  string_file output;
+  bfd_print_error (print_error_callback, &output, fmt, ap);
+  std::string str = output.release ();
 
-  if (increment_bfd_error_count (std::move (str)) > 1)
+  if (increment_bfd_error_count (str) > 1)
     return;
 
-  /* We must call the BFD mechanism for printing format strings since
-     it supports additional format specifiers that GDB's vwarning() doesn't
-     recognize.  It also outputs additional text, i.e. "BFD: ", which
-     makes it clear that it's a BFD warning/error.  */
-  (*default_bfd_error_handler) (fmt, ap);
+  warning ("%s", str.c_str ());
 }
 
 /* See gdb_bfd.h.  */
@@ -1263,5 +1272,5 @@ When non-zero, bfd cache specific debugging is enabled."),
 			   &setdebuglist, &showdebuglist);
 
   /* Hook the BFD error/warning handler to limit amount of output.  */
-  default_bfd_error_handler = bfd_set_error_handler (gdb_bfd_error_handler);
+  bfd_set_error_handler (gdb_bfd_error_handler);
 }
-- 
2.43.0

Re: [PATCH] Rewrite gdb_bfd_error_handler

[Tom Tromey]

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

Tom> This patch rewrites gdb_bfd_error_handler to use 'bfd_print_error' to
Tom> generate the text of the warning, and then emits it using 'warning'.
Tom> The current code in the tree is a bit wrong because it may do the
Tom> wrong thing when BFD uses ones of its printf extensions.

Tom> This also adds locking to increment_bfd_error_count.  This is
Tom> important now that some BFD operations can be done on worker threads.

Tom> This approach makes it simpler for worker threads to intercept any
Tom> messages.

Tom> Regression tested on x86-64 Fedora 38.

I'm checking this in.

Tom