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

[PATCH 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.)

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

 gdb/gdb_bfd.c                            |  71 ++++++++
 gdb/testsuite/gdb.base/bfd-errors-lib.c  |  44 +++++
 gdb/testsuite/gdb.base/bfd-errors-main.c |  31 ++++
 gdb/testsuite/gdb.base/bfd-errors.exp    | 211 +++++++++++++++++++++++
 4 files changed, 357 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/bfd-errors-lib.c
 create mode 100644 gdb/testsuite/gdb.base/bfd-errors-main.c
 create mode 100644 gdb/testsuite/gdb.base/bfd-errors.exp

-- 
2.37.2

[PATCH 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 a 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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 6c03ae5ef05..e74d649ea16 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 <map>
 
 /* An object of this type is stored in the section's user data when
    mapping a section.  */
@@ -1125,6 +1126,73 @@ 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::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;
+  if (map.find (str) == map.end ())
+    {
+      map[str] = 0;
+    }
+  return ++map[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 (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 +1225,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.2

[PATCH 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-main.c |  31 ++++
 gdb/testsuite/gdb.base/bfd-errors.exp    | 211 +++++++++++++++++++++++
 3 files changed, 286 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/bfd-errors-lib.c
 create mode 100644 gdb/testsuite/gdb.base/bfd-errors-main.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-main.c b/gdb/testsuite/gdb.base/bfd-errors-main.c
new file mode 100644
index 00000000000..153f29704eb
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bfd-errors-main.c
@@ -0,0 +1,31 @@
+/* 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>
+
+extern void foo1();
+extern void foo2();
+extern void foo3();
+extern void foo4();
+
+int main ()
+{
+  printf ("in main\n");
+  foo1 ();
+  foo2 ();
+  foo3 ();
+  foo4 ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/bfd-errors.exp b/gdb/testsuite/gdb.base/bfd-errors.exp
new file mode 100644
index 00000000000..6d5a9428dc0
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bfd-errors.exp
@@ -0,0 +1,211 @@
+# 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 sets a catchpoint on the shared library load
+# operation and GDB is run to that catchpoint.  On the way to running
+# to the catchpoint, the test observes and records the BFD errors that
+# were output.  Finally, data collected while running to the
+# catchpoint 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 [list debug ldflags=-Wl,-Bsymbolic]
+
+# Main executable names and flags:
+set main_basename ${::gdb_test_file_name}-main
+set srcfile ${srcdir}/${subdir}/${main_basename}.c
+set binfile [standard_output_file ${main_basename}]
+set bin_flags [list debug shlib=${binfile_lib}]
+
+# Compile shared library and main executable:
+if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
+     || [gdb_compile ${srcfile} ${binfile} executable $bin_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"] } {
+    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 {[run_on_host \
+	"objcopy dump-section" \
+	${objcopy_program} \
+	"--dump-section .dynsym=${binfile_lib}.dynsym --dump-section .dynstr=${binfile_lib}.dynstr ${binfile_lib}"]} {
+    untested "failed objcopy dump-section"
+    return -1
+}
+
+# Determine length of .dynstr.  We'll use the length for creating invalid
+# offsets into .dynstr.
+set dynstr_fp [open ${binfile_lib}.dynstr r]
+fconfigure $dynstr_fp -translation binary
+set dynstr_len [string length [read $dynstr_fp]]
+close $dynstr_fp
+
+# 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 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 {[run_on_host \
+	"objcopy update-section" \
+	${objcopy_program} \
+	"--update-section .dynsym=${binfile_lib}.dynsym ${binfile_lib}"]} {
+    untested "failed objcopy update-section"
+    return -1
+}
+
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+gdb_load_shlib $binfile_lib
+
+# We don't run to main because doing so will sometimes cause a SIGSEGV
+# in the dynamic linker/loader due to using a mangled shared object. 
+# Instead, we'll stop at the shared library load.
+gdb_test "catch load ${lib_basename}" "Catchpoint.*load.*"
+
+gdb_run_cmd
+
+# Count number of distinct BFD error messages via 'bfd_error_count'
+# array while running to the catchpoint.
+set testname "run to catchpoint"
+gdb_test_multiple "" $testname {
+    -re "(BFD:\[^\r\n\]*)\[\r\n\]+" {
+	incr bfd_error_count($expect_out(1,string))
+	exp_continue
+    }
+    -re "Catchpoint .*${lib_basename}.*$gdb_prompt $" {
+	pass $testname
+    }
+}
+
+# Examine counts recorded in the 'bfd_error_count' array to see if any
+# message was printed multiple times.
+set testname "consolidated bfd errors"
+set more_than_one 0
+foreach index [array names bfd_error_count] {
+    if { $bfd_error_count($index) > 1 } {
+	incr more_than_one
+    }
+}
+if { $more_than_one > 0 } {
+    fail $testname
+} else {
+    pass $testname
+}
+
+# There should have been at least two distinct BFD errors printed.
+set testname "print all unique bfd errors"
+if { [array size bfd_error_count] >= 2 } {
+    pass $testname
+} else {
+    fail $testname
+}
+
+gdb_exit
+
+return 0
-- 
2.37.2

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

[Lancelot SIX]

Hi Kevin

On Fri, Sep 02, 2022 at 05:47:58PM -0700, Kevin Buettner via Gdb-patches wrote:
> 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 a 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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index 6c03ae5ef05..e74d649ea16 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 <map>
>  
>  /* An object of this type is stored in the section's user data when
>     mapping a section.  */
> @@ -1125,6 +1126,73 @@ 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::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;
> +  if (map.find (str) == map.end ())
> +    {
> +      map[str] = 0;
> +    }
> +  return ++map[str];

I think this can all be simplified.

If `str` is not a key in `map`, `map[str]` will default initialize a
value in the map (in this case 0-initialize as it is an unsigned long)
before returning the reference to it.

https://en.cppreference.com/w/cpp/container/map/operator_at says:

  If an insertion is performed, the mapped value is value-initialized
  (default-constructed for class types, zero-initialized otherwise) and
  a reference to it is returned.

As a consequence, this block can all be summed up as:

    return ++map[str];
           | |
	   |  - 1) Fetch existing value, or 0-initialize one and return
	   |       it
	   |
	    - 2) Do the pre-increment.

If you prefer a more explicit option, std::map::insert inserts an
element in a map if not already present, and return a pair containing:
  - An iterator to the newly inserted element if an insertion took
    place, to the element which was already in the map otherwise
  - A bool indicating if an insertion took place.

The above could be re-written:

    return ++map.insert ({str, 0ul}).first->second;

This would avoid having to do the lookup twice to increment a counter
(the initial map::find and then map::operator[]), or three times to
initialize if the counter needs to be initialized (map::find and then
map::operator[] twice).

Best,
Lancelot.

> +}
> +
> +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 (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 +1225,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.2
> 

Re: [PATCH 2/2] BFD error message suppression test case

[Lancelot SIX]

Hi,

When applying this patch, I have:

    Applying: BFD error message suppression test case
    [...]/rebase-apply/patch:269: trailing whitespace.
    # in the dynamic linker/loader due to using a mangled shared object. 
    warning: 1 line adds whitespace errors.

Also, on my systems (ubuntu 22.04 and ubuntu 20.04), the test fails:

    (gdb) catch load bfd-errors-lib
    Catchpoint 1 (load)
    (gdb) PASS: gdb.base/bfd-errors.exp: catch load bfd-errors-lib
    run 
    Starting program: .../gdb/testsuite/outputs/gdb.base/bfd-errors/bfd-errors-main 
    .../gdb/testsuite/outputs/gdb.base/bfd-errors/bfd-errors-main: symbol lookup error: .../gdb/testsuite/outputs/gdb.base/bfd-errors/bfd-errors-main: undefined symbol: foo4
    [Inferior 1 (process 19165) exited with code 0177]
    (gdb) FAIL: gdb.base/bfd-errors.exp: run to catchpoint (the program exited)
    PASS: gdb.base/bfd-errors.exp: consolidated bfd errors
    FAIL: gdb.base/bfd-errors.exp: print all unique bfd error

I have not really looked at the errors in detail.

I also have minor comments inlined in the patch.

On Fri, Sep 02, 2022 at 05:47:59PM -0700, Kevin Buettner via Gdb-patches wrote:
> 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-main.c |  31 ++++
>  gdb/testsuite/gdb.base/bfd-errors.exp    | 211 +++++++++++++++++++++++
>  3 files changed, 286 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/bfd-errors-lib.c
>  create mode 100644 gdb/testsuite/gdb.base/bfd-errors-main.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-main.c b/gdb/testsuite/gdb.base/bfd-errors-main.c
> new file mode 100644
> index 00000000000..153f29704eb
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/bfd-errors-main.c
> @@ -0,0 +1,31 @@
> +/* 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>
> +
> +extern void foo1();
> +extern void foo2();
> +extern void foo3();
> +extern void foo4();
> +
> +int main ()
> +{
> +  printf ("in main\n");
> +  foo1 ();
> +  foo2 ();
> +  foo3 ();
> +  foo4 ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/bfd-errors.exp b/gdb/testsuite/gdb.base/bfd-errors.exp
> new file mode 100644
> index 00000000000..6d5a9428dc0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/bfd-errors.exp
> @@ -0,0 +1,211 @@
> +# 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 sets a catchpoint on the shared library load
> +# operation and GDB is run to that catchpoint.  On the way to running
> +# to the catchpoint, the test observes and records the BFD errors that
> +# were output.  Finally, data collected while running to the
> +# catchpoint 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 [list debug ldflags=-Wl,-Bsymbolic]
> +
> +# Main executable names and flags:
> +set main_basename ${::gdb_test_file_name}-main
> +set srcfile ${srcdir}/${subdir}/${main_basename}.c
> +set binfile [standard_output_file ${main_basename}]
> +set bin_flags [list debug shlib=${binfile_lib}]
> +
> +# Compile shared library and main executable:
> +if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
> +     || [gdb_compile ${srcfile} ${binfile} executable $bin_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"] } {
> +    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 {[run_on_host \
> +	"objcopy dump-section" \
> +	${objcopy_program} \
> +	"--dump-section .dynsym=${binfile_lib}.dynsym --dump-section .dynstr=${binfile_lib}.dynstr ${binfile_lib}"]} {
> +    untested "failed objcopy dump-section"
> +    return -1
> +}
> +
> +# Determine length of .dynstr.  We'll use the length for creating invalid
> +# offsets into .dynstr.
> +set dynstr_fp [open ${binfile_lib}.dynstr r]
> +fconfigure $dynstr_fp -translation binary
> +set dynstr_len [string length [read $dynstr_fp]]
> +close $dynstr_fp
> +
> +# 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 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 {[run_on_host \
> +	"objcopy update-section" \
> +	${objcopy_program} \
> +	"--update-section .dynsym=${binfile_lib}.dynsym ${binfile_lib}"]} {
> +    untested "failed objcopy update-section"
> +    return -1
> +}
> +
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}
> +gdb_load_shlib $binfile_lib
> +
> +# We don't run to main because doing so will sometimes cause a SIGSEGV
> +# in the dynamic linker/loader due to using a mangled shared object. 
> +# Instead, we'll stop at the shared library load.
> +gdb_test "catch load ${lib_basename}" "Catchpoint.*load.*"
> +
> +gdb_run_cmd
> +
> +# Count number of distinct BFD error messages via 'bfd_error_count'
> +# array while running to the catchpoint.
> +set testname "run to catchpoint"
> +gdb_test_multiple "" $testname {
> +    -re "(BFD:\[^\r\n\]*)\[\r\n\]+" {
> +	incr bfd_error_count($expect_out(1,string))
> +	exp_continue
> +    }
> +    -re "Catchpoint .*${lib_basename}.*$gdb_prompt $" {
> +	pass $testname

You could use $gdb_test_name here.  This would avoid the need of the
testname variable.

> +    }
> +}
> +
> +# Examine counts recorded in the 'bfd_error_count' array to see if any
> +# message was printed multiple times.
> +set testname "consolidated bfd errors"
> +set more_than_one 0
> +foreach index [array names bfd_error_count] {
> +    if { $bfd_error_count($index) > 1 } {
> +	incr more_than_one
> +    }
> +}
> +if { $more_than_one > 0 } {
> +    fail $testname
> +} else {
> +    pass $testname
> +}
> +

I guess you could use

    gdb_assert { $more_than_one > 0 } "consolidated bfd errors"

> +# There should have been at least two distinct BFD errors printed.
> +set testname "print all unique bfd errors"
> +if { [array size bfd_error_count] >= 2 } {
> +    pass $testname
> +} else {
> +    fail $testname
> +}

Similarly, I think gdb_assert would be more concise.

Best,
Lancelot.

> +
> +gdb_exit
> +
> +return 0
> -- 
> 2.37.2
> 

Re: [PATCH 2/2] BFD error message suppression test case

[Kevin Buettner]

On Tue, 6 Sep 2022 10:29:08 +0000
Lancelot SIX <lsix@lancelotsix.com> wrote:

> When applying this patch, I have:
> 
>     Applying: BFD error message suppression test case
>     [...]/rebase-apply/patch:269: trailing whitespace.
>     # in the dynamic linker/loader due to using a mangled shared object. 
>     warning: 1 line adds whitespace errors.

Thanks.  I've fixed this in my local sources.

> Also, on my systems (ubuntu 22.04 and ubuntu 20.04), the test fails:
> 
>     (gdb) catch load bfd-errors-lib
>     Catchpoint 1 (load)
>     (gdb) PASS: gdb.base/bfd-errors.exp: catch load bfd-errors-lib
>     run 
>     Starting program: .../gdb/testsuite/outputs/gdb.base/bfd-errors/bfd-errors-main 
>     .../gdb/testsuite/outputs/gdb.base/bfd-errors/bfd-errors-main: symbol lookup error: .../gdb/testsuite/outputs/gdb.base/bfd-errors/bfd-errors-main: undefined symbol: foo4
>     [Inferior 1 (process 19165) exited with code 0177]
>     (gdb) FAIL: gdb.base/bfd-errors.exp: run to catchpoint (the program exited)
>     PASS: gdb.base/bfd-errors.exp: consolidated bfd errors
>     FAIL: gdb.base/bfd-errors.exp: print all unique bfd error

I've reproduced this on Ubuntu 22.04.

I was concerned that setting a catchpoint on the shared library load
might be fragile.  Sadly, it seems that it is.  I was expecting it to break
differently than what we're seeing on Ubuntu though.

I've perhaps come up with a different approach using 'add-symbol-file'
which I'll (try to) put into a v2 series.  If it works, this will take
the dynamic linker entirely out of the picture and also won't rely on
a load actually being caught.

> I also have minor comments inlined in the patch.

Thanks - I'll make those adjustments in the v2 series too.

Kevin

P.S. Thanks, too, for your comments on part 1 of this series.