From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 234373858D28 for ; Wed, 7 Sep 2022 18:53:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 234373858D28 Received: from [172.16.0.64] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 886A31E13B; Wed, 7 Sep 2022 14:53:30 -0400 (EDT) Message-ID: <5494e3a9-7e1c-7592-90b7-e71bd9375620@simark.ca> Date: Wed, 7 Sep 2022 14:53:29 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH v2 1/2] Suppress printing of superfluous BFD error messages Content-Language: fr To: Kevin Buettner , gdb-patches@sourceware.org Cc: lsix@lancelotsix.com References: <20220907173050.8739-1-kevinb@redhat.com> <20220907173050.8739-2-kevinb@redhat.com> From: Simon Marchi In-Reply-To: <20220907173050.8739-2-kevinb@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Sep 2022 18:53:35 -0000 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 > > /* 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 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::key 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