[PATCH v3 0/2] Suppress printing of superfluous BFD error messages

[PATCH v3 0/2] Suppress printing of superfluous BFD error messages

[Kevin Buettner]

Tools which use the BFD library will output error messages of the
form "BFD: some messsage" when a problem with the file upon which it
operating is found.  E.g. an actual message (modulo some shortening
of the pathname) from the test case in this series is:

BFD: bfd-errors-lib.so: invalid string offset 1154 >= 154 for section `.dynstr'

For some problems with executable files or libraries, BFD will
attempt to output many identical messages.  The first patch in this
series adds code to GDB to suppress messages which are identical to
earlier messages that have already been printed.

(The above blurb was adapted from a comment in the test case.)

The V2 patch addresses concerns noted by Lancelot SIX:

For part 1, I've used one of Lancelot's suggestions for simplifying
the implementation of increment_bfd_error_count() in gdb_bfd.c.

Regarding the test case, Lancelot found that it failed when tested on
Ubuntu.  It turned out that attempting to run to a catchpoint whilst
using a mangled shared library was fragile.  I rewrote parts of the
test to instead use 'add-symbol-file -readnow' to load symbols from
the mangled shared object into GDB.  Also, since the main program is
no longer needed, I removed it and all references to in in the .exp
file.  Lancelot suggested several other improvements which I also put
into place.

This V3 patch addresses concerns from Simon Marchi:

In part 1, I changed the type of the associative array used to keep
track of BFD messages from 'map' to 'unordered_map', changed the
case of BFD ('bfd' -> 'BFD') in a couple of comments, and used
std::move to convert the error string to an xvalue.

The changes to part 2 include adding a 'close' along an error path,
changing the objcopy invocations to use 'catch "exec ..."' instead
of run_on_host, use of 'file size' instead of opening and reading
the file to determine its size, and the use of 'clean_restart' in
place of discrete invocations of 'gdb_start' and 'gdb_reinitialize_dir'.

Kevin Buettner (2):
  Suppress printing of superfluous BFD error messages
  BFD error message suppression test case

 gdb/gdb_bfd.c                           |  67 +++++++++
 gdb/testsuite/gdb.base/bfd-errors-lib.c |  44 ++++++
 gdb/testsuite/gdb.base/bfd-errors.exp   | 185 ++++++++++++++++++++++++
 3 files changed, 296 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/bfd-errors-lib.c
 create mode 100644 gdb/testsuite/gdb.base/bfd-errors.exp

-- 
2.37.3

[PATCH v3 1/2] Suppress printing of superfluous BFD error messages

[Kevin Buettner]

This commit adds a hook to the BFD error handler for suppressing
identical messages which have been output once already.

It's motivated by this Fedora bug...

https://bugzilla.redhat.com/show_bug.cgi?id=2083315

...in which over 900,000 BFD error messages are output when attaching
to firefox.  From the bug report, the messages all say:

BFD: /usr/lib/debug/usr/lib64/firefox/libxul.so-100.0-2.fc35.x86_64.debug: attempt to load strings from a non-string section (number 38)

Since there's no (additional) context which might assist the user in
determining what's wrong, there's really no point in outputting more
than one message.  Of course, if BFD should output some
other/different message, it should be output too, but all future
messages identical to those already output should be suppressed.

For the firefox problem, it turned out that there were only 37
sections, but something was referring to section #38.  I haven't
investigated further to find out how this came to be.

Despite this problem, useful debugging might still be done, especially
if the user doesn't care about debugging the problematic library.

If it turns out that knowing the quantity of messages might be useful,
I've implemented the suppression mechanism by keeping a count of each
identical message.  A new GDB command, perhaps a 'maintenance'
command, could be added to print out each message along with the
count.  I haven't implemented this though because I'm not convinced of
its utility.  Also, the BFD message printer has support for BFD-
specific format specifiers.  The BFD message strings that GDB stores
in its map are sufficient for distinguishing messages from each
other, but are not identical to those output by BFD's default error
handler.  So, that problem would need to be solved too.
---
 gdb/gdb_bfd.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 6c03ae5ef05..6299148d419 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -33,6 +33,7 @@
 #include "gdb/fileio.h"
 #include "inferior.h"
 #include "cli/cli-style.h"
+#include <unordered_map>
 
 /* An object of this type is stored in the section's user data when
    mapping a section.  */
@@ -1125,6 +1126,69 @@ maintenance_info_bfds (const char *arg, int from_tty)
   htab_traverse (all_bfds, print_one_bfd, uiout);
 }
 
+/* BFD related per-inferior data.  */
+
+struct bfd_inferior_data
+{
+  std::unordered_map<std::string, unsigned long> bfd_error_string_counts;
+};
+
+/* Per-inferior data key.  */
+
+static const registry<inferior>::key<bfd_inferior_data> bfd_inferior_data_key;
+
+/* Fetch per-inferior BFD data.  It always returns a valid pointer to
+   a bfd_inferior_data struct.  */
+
+static struct bfd_inferior_data *
+get_bfd_inferior_data (struct inferior *inf)
+{
+  struct bfd_inferior_data *data;
+
+  data = bfd_inferior_data_key.get (inf);
+  if (data == nullptr)
+    data = bfd_inferior_data_key.emplace (inf);
+
+  return data;
+}
+
+/* Increment the BFD error count for STR and return the updated
+   count.  */
+
+static unsigned long
+increment_bfd_error_count (std::string str)
+{
+  struct bfd_inferior_data *bid = get_bfd_inferior_data (current_inferior ());
+
+  auto &map = bid->bfd_error_string_counts;
+  return ++map[std::move (str)];
+}
+
+static bfd_error_handler_type default_bfd_error_handler;
+
+/* Define a BFD error handler which will suppress the printing of
+   messages which have been printed once already.  This is done on a
+   per-inferior basis.  */
+
+static void
+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);
+
+  if (increment_bfd_error_count (std::move (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);
+}
+
 void _initialize_gdb_bfd ();
 void
 _initialize_gdb_bfd ()
@@ -1157,4 +1221,7 @@ When non-zero, bfd cache specific debugging is enabled."),
 			   NULL,
 			   &show_bfd_cache_debug,
 			   &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);
 }
-- 
2.37.3

[PATCH v3 2/2] BFD error message suppression test case

[Kevin Buettner]

This commit adds a GDB test case which tests GDB's BFD error handler
hook for suppressing output of all but the first identical messages.

See the comment at the beginning of bfd-errors.exp for details about
this new test.

I've tested this test for both 32- and 64-bit ELF files and also
on both little endian and big endian machines.  It also works for
both native and remote targets.  The only major restriction is that
it only works for ELF targets.
---
 gdb/testsuite/gdb.base/bfd-errors-lib.c |  44 ++++++
 gdb/testsuite/gdb.base/bfd-errors.exp   | 185 ++++++++++++++++++++++++
 2 files changed, 229 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/bfd-errors-lib.c
 create mode 100644 gdb/testsuite/gdb.base/bfd-errors.exp

diff --git a/gdb/testsuite/gdb.base/bfd-errors-lib.c b/gdb/testsuite/gdb.base/bfd-errors-lib.c
new file mode 100644
index 00000000000..2b582d14c58
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bfd-errors-lib.c
@@ -0,0 +1,44 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <stdio.h>
+
+void
+foo1 ()
+{
+  printf ("foo1 in lib\n");
+  return;
+}
+
+void
+foo2()
+{
+  printf ("foo2 in lib\n");
+  return;
+}
+
+void
+foo3()
+{
+  printf ("foo3 in lib\n");
+  return;
+}
+
+void
+foo4()
+{
+  printf ("foo4 in lib\n");
+  return;
+}
diff --git a/gdb/testsuite/gdb.base/bfd-errors.exp b/gdb/testsuite/gdb.base/bfd-errors.exp
new file mode 100644
index 00000000000..e436e40ba6e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bfd-errors.exp
@@ -0,0 +1,185 @@
+# Copyright 2022 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Tools which use the BFD library will output error messages of the
+# form "BFD: some messsage" when a problem with the file upon which it
+# operating is found.  E.g. an actual message (modulo some shortening
+# of the pathname) from this test is:
+#
+# BFD: bfd-errors-lib.so: invalid string offset 1154 >= 154 for section `.dynstr'
+#
+# For some problems with executable files or libraries, BFD will
+# attempt to output many identical messages.  Code has been added to
+# GDB to suppress messages which are identical to earlier messages
+# that have already been printed.
+#
+# This test makes sure that (all but the first) identical BFD messages
+# are suppresssed and also that differing messages are output at least
+# once.
+#
+# To accomplish this, a shared object with at least four symbols is
+# created.  The .dynsym section is extracted and offsets which should
+# refer to strings in the .dynstr section are changed to be
+# larger than the size of the .dynstr section.  Only two (different)
+# offsets are used; thus BFD will attempt to output at least two pairs
+# of identical messages.  (And it would do this too if not intercepted
+# by the hook placed by GDB.)  After modifying the extracted section,
+# the mangled section is placed back into the shared object.
+#
+# This test then loads the shared library's symbol table (and other
+# debug info) using the 'add-symbol-file' command.  While doing this,
+# the test observes and records the BFD errors that were output. 
+# Finally, data collected while adding the shared library symbols are
+# examined to make sure that identical messages were suppressed while
+# also making sure that at least two messages have been printed.
+
+# This test can't be run on targets lacking shared library support
+# or for non-ELF targets.
+if { [skip_shlib_tests] || ![is_elf_target] } {
+    return 0
+}
+
+# Library file names and flags:
+set lib_basename ${::gdb_test_file_name}-lib
+set srcfile_lib ${srcdir}/${subdir}/${lib_basename}.c
+set binfile_lib [standard_output_file ${lib_basename}.so]
+set lib_flags debug
+
+# Compile shared library:
+if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+# Open the shared library and determine some basic facts.  The key
+# things that we need to learn are 1) whether the solib is 32-bit or
+# 64-bit ELF file, and 2) the endianness.
+set solib_fp [open ${binfile_lib} r]
+fconfigure $solib_fp -translation binary
+
+# Read and check EI_MAG to verify that it's really an ELF file.
+set data [read $solib_fp 4]
+if { ![string equal $data "\x7fELF"] } {
+    close $solib_fp
+    untested "shared library is not an ELF file"
+    return -1
+}
+
+# Read EI_CLASS for ELF32 versus ELF64.
+set data [read $solib_fp 1]
+set is_elf64 [string equal $data "\x02"]
+
+# Read EI_DATA to determine data encoding (byte order).
+set data [read $solib_fp 1]
+set is_big_endian [string equal $data "\x02"]
+
+close $solib_fp
+
+set objcopy_program [gdb_find_objcopy]
+
+# Extract the .dynsym and .dynstr section from the shared object.
+if { [catch "exec $objcopy_program \
+              --dump-section .dynsym=${binfile_lib}.dynsym \
+	      --dump-section .dynstr=${binfile_lib}.dynstr \
+	      ${binfile_lib}" output] } {
+    untested "failed objcopy dump-section"
+    verbose -log "objcopy output: $output"
+    return -1
+}
+
+# Determine length of .dynstr.  We'll use the length for creating invalid
+# offsets into .dynstr.
+set dynstr_len [file size ${binfile_lib}.dynstr]
+
+# Open the file containing .dynsym and determine its length.  In this
+# case, we want to know the length in order to compute the total number
+# of symbols that it contains.  We also leave the file open for a
+# while so that we can write invalid offsets to it.
+set dynsym_fp [open ${binfile_lib}.dynsym r+]
+fconfigure $dynsym_fp -translation binary
+set dynsym_len [string length [read $dynsym_fp]]
+
+# SZ is the size of the Elf32_Sym / Elf64_Sym struct.  OFF is the
+# offset into the file.  CNT is one greater than the number of symbols
+# left to mangle.  Note that, in the loop below, the first symbol is
+# skipped.  This is intentional since the first symbol is defined by
+# the ELF specification to be the undefined symbol.
+set off 0
+if { $is_elf64 } {
+    set sz 24
+} else {
+    set sz 16
+}
+set cnt [expr $dynsym_len / $sz]
+
+# Create 32-bit patterns (bad offsets) to write into the st_name area.
+if { $is_big_endian } {
+    set pat(0) [binary format I [expr $dynstr_len + 1000]]
+    set pat(1) [binary format I [expr $dynstr_len + 2000]]
+} else {
+    set pat(0) [binary format i [expr $dynstr_len + 1000]]
+    set pat(1) [binary format i [expr $dynstr_len + 2000]]
+}
+
+# Mangle st_name for the symbols following the first (STN_UNDEF) entry.
+while { [incr cnt -1] > 0 } {
+    seek $dynsym_fp [incr off $sz]
+    puts $dynsym_fp $pat([expr $cnt % 2])
+}
+
+close $dynsym_fp
+
+# Replace .dynsym section in shared object with the mangled version.
+if { [catch "exec $objcopy_program \
+               --update-section .dynsym=${binfile_lib}.dynsym \
+	       ${binfile_lib}" output] } {
+    untested "failed objcopy update-section"
+    verbose -log "objcopy output: $output"
+    return -1
+}
+
+clean_restart
+
+# Count number of distinct BFD error messages via 'bfd_error_count'
+# array while using add-symbol-file to "load" the shared library.
+gdb_test_multiple "add-symbol-file -readnow $binfile_lib" \
+                  "load library with add-symbol-file" {
+    -re "add symbol table from file.*\(y or n\)" {
+	send_gdb "y\n" answer
+	exp_continue
+    }
+    -re "(BFD:\[^\r\n\]*)\[\r\n\]+" {
+	incr bfd_error_count($expect_out(1,string))
+	exp_continue
+    }
+    -re "Expanding full symbols from.*$gdb_prompt $" {
+	pass $gdb_test_name
+    }
+}
+
+# Examine counts recorded in the 'bfd_error_count' array to see if any
+# message was printed multiple times.
+set more_than_one 0
+foreach index [array names bfd_error_count] {
+    if { $bfd_error_count($index) > 1 } {
+	incr more_than_one
+    }
+}
+gdb_assert { $more_than_one == 0 } "consolidated bfd errors"
+
+# There should have been at least two distinct BFD errors printed.
+gdb_assert { [array size bfd_error_count] >= 2 } "print all unique bfd errors"
+
+gdb_exit
+return 0
-- 
2.37.3

Re: [PATCH v3 0/2] Suppress printing of superfluous BFD error messages

[Kevin Buettner]

On Wed,  7 Sep 2022 18:58:10 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> Kevin Buettner (2):
>   Suppress printing of superfluous BFD error messages
>   BFD error message suppression test case
> 
>  gdb/gdb_bfd.c                           |  67 +++++++++
>  gdb/testsuite/gdb.base/bfd-errors-lib.c |  44 ++++++
>  gdb/testsuite/gdb.base/bfd-errors.exp   | 185 ++++++++++++++++++++++++
>  3 files changed, 296 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/bfd-errors-lib.c
>  create mode 100644 gdb/testsuite/gdb.base/bfd-errors.exp

I've pushed this series.