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

[Lancelot SIX <lsix@lancelotsix.com>]

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
>