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

[Simon Marchi]

On 9/7/22 13:30, 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.

That looks good to me.  Some nits below.

> ---
>  gdb/gdb_bfd.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index 6c03ae5ef05..aa75d7bedcf 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,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::map<std::string, unsigned long> bfd_error_string_counts;

You could use unordered_map, since we don't care about ordering of keys.

> +};
> +
> +/* 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

bfd -> BFD

> +   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[str];

You could std::move `str` here...

> +}
> +
> +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)

... and std::move str here too.

> +    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.  */

bfd -> BFD

Simon