From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id aXxPFXMcF2NOajQAWB0awg (envelope-from ) for ; Tue, 06 Sep 2022 06:09:55 -0400 Received: by simark.ca (Postfix, from userid 112) id 4C62B1E4A7; Tue, 6 Sep 2022 06:09:55 -0400 (EDT) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=G0pUm6aC; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id AF1FB1E13B for ; Tue, 6 Sep 2022 06:09:54 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 99BD33851408 for ; Tue, 6 Sep 2022 10:09:52 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 99BD33851408 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1662458992; bh=Hv4ZBf6Q0nK+JyycoHnQ2+6lddPrBzmrVdjIqRQzPHk=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=G0pUm6aCiQ97m3H08pxPdHXBnzKvffbOvQLTEkqHqEF24e6o7DTrZIsGBzAa7A4N3 859ZgQgisvjtXWtGXYSvnQb8GtXqkbChBGe5l8lTb7siaZbJdSEUTo3F8bwdQ3M0cU 8dmq0FiRv/tKotHEjL8e9vgykhQ3rsOThGu/mu3o= Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 9B3AA38582AD for ; Tue, 6 Sep 2022 10:09:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9B3AA38582AD Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 4AD59891A2; Tue, 6 Sep 2022 10:09:30 +0000 (UTC) Date: Tue, 6 Sep 2022 10:09:24 +0000 To: Kevin Buettner Subject: Re: [PATCH 1/2] Suppress printing of superfluous BFD error messages Message-ID: <20220906100924.7v3flonmeawrqohv@ubuntu.lan> References: <20220903004759.2082950-1-kevinb@redhat.com> <20220903004759.2082950-2-kevinb@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220903004759.2082950-2-kevinb@redhat.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Tue, 06 Sep 2022 10:09:30 +0000 (UTC) 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: , From: Lancelot SIX via Gdb-patches Reply-To: Lancelot SIX Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" 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 > > /* 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 bfd_error_string_counts; > +}; > + > +/* 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 > + 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 >