Message ID | 20230104212242.545914-1-simon.marchi@polymtl.ca |
---|---|
State | Committed |
Commit | 7e0bd9ea7e27243661a1ca0291f59ff6c95e692d |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5CD813858C2D for <patchwork@sourceware.org>; Wed, 4 Jan 2023 21:23:14 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5CD813858C2D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1672867394; bh=HIT9245yqMtMthgnIjgdOSZLcFKBciNNd+7P4dve/Ms=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=btrkZSDob5/Z682h5c+zcjmBNr2RtNr15tmBSIk96Vd/eSmA/413pTKIKxdowc/6e Fzkz/CQWKxTb8c876ahsLLpTOY6JtLWwxgrLRT2+8slehrV3uwLu0vG5RTJ3LuMoEA RUMkzhy8gfreUZFQoonjMq68QAqxdNJlRSnPtm3U= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 451703858D35 for <gdb-patches@sourceware.org>; Wed, 4 Jan 2023 21:22:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 451703858D35 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 304LMheC013652 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 4 Jan 2023 16:22:47 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 304LMheC013652 Received: from simark.localdomain (unknown [217.28.27.60]) (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 ESMTPSA id 016C01E112; Wed, 4 Jan 2023 16:22:42 -0500 (EST) To: gdb-patches@sourceware.org Cc: Andrew Burgess <aburgess@redhat.com>, Simon Marchi <simon.marchi@polymtl.ca> Subject: [PATCH] gdbsupport: fix scoped_debug_start_end's move constructor Date: Wed, 4 Jan 2023 16:22:42 -0500 Message-Id: <20230104212242.545914-1-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.39.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 4 Jan 2023 21:22:43 +0000 X-Spam-Status: No, score=-3189.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP, WEIRD_PORT 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 <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Simon Marchi <simon.marchi@polymtl.ca> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
gdbsupport: fix scoped_debug_start_end's move constructor
|
|
Commit Message
Simon Marchi
Jan. 4, 2023, 9:22 p.m. UTC
I spotted a problem with scoped_debug_start_end's move constructor. When constructing a scoped_debug_start_end through it, it doesn't disable the moved-from object, meaning there are now two objects that will do the side-effects of decrementing the debug_print_depth global and printing the "end" message. Decrementing the debug_print_depth global twice is actually problematic, because the increments and decrements get out of sync, meaning we should hit this assertion, in theory: gdb_assert (debug_print_depth > 0); However, in practice, we don't see that. This is because despite the move constructor being required for this to compile: template<typename PT> static inline scoped_debug_start_end<PT &> ATTRIBUTE_NULL_PRINTF (6, 7) make_scoped_debug_start_end (PT &&pred, const char *module, const char *func, const char *start_prefix, const char *end_prefix, const char *fmt, ...) { va_list args; va_start (args, fmt); auto res = scoped_debug_start_end<PT &> (pred, module, func, start_prefix, end_prefix, fmt, args); va_end (args); return res; } ... it is never actually called, because compilers elide the move constructors all the way (the scoped_debug_start_end gets constructed directly in the instance of the top-level caller). To confirm this, I built GDB with -fno-elide-constructors, and now I see it: /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:147: internal-error: ~scoped_debug_start_end: Assertion `debug_print_depth > 0' failed. #9 0x00005614ba5f17c3 in internal_error_loc (file=0x5614b8749960 "/home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h", line=147, fmt=0x5614b8733fa0 "%s: Assertion `%s' failed.") at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:58 #10 0x00005614b8e1b2e5 in scoped_debug_start_end<bool&>::~scoped_debug_start_end (this=0x7ffc6c5e7b40, __in_chrg=<optimized out>) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:147 #11 0x00005614b96dbe34 in make_scoped_debug_start_end<bool&> (pred=@0x5614baad7200: true, module=0x5614b891d840 "infrun", func=0x5614b891d800 "infrun_debug_show_threads", start_prefix=0x5614b891d7c0 "enter", end_prefix=0x5614b891d780 "exit", fmt=0x0) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:235 Fix this by adding an m_disabled field to scoped_debug_start_end, and setting it in the move constructor. Change-Id: Ie5213269c584837f751d2d11de831f45ae4a899f --- gdbsupport/common-debug.h | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) base-commit: aa036eccf094c4d85b953cf1cc2892f0ff746fd9
Comments
On 1/4/23 16:22, Simon Marchi wrote: > I spotted a problem with scoped_debug_start_end's move constructor. > When constructing a scoped_debug_start_end through it, it doesn't > disable the moved-from object, meaning there are now two objects that > will do the side-effects of decrementing the debug_print_depth global > and printing the "end" message. Decrementing the debug_print_depth > global twice is actually problematic, because the increments and > decrements get out of sync, meaning we should hit this assertion, in > theory: > > gdb_assert (debug_print_depth > 0); > > However, in practice, we don't see that. This is because despite the > move constructor being required for this to compile: > > template<typename PT> > static inline scoped_debug_start_end<PT &> ATTRIBUTE_NULL_PRINTF (6, 7) > make_scoped_debug_start_end (PT &&pred, const char *module, const char *func, > const char *start_prefix, > const char *end_prefix, const char *fmt, ...) > { > va_list args; > va_start (args, fmt); > auto res = scoped_debug_start_end<PT &> (pred, module, func, start_prefix, > end_prefix, fmt, args); > va_end (args); > > return res; > } > > ... it is never actually called, because compilers elide the move > constructors all the way (the scoped_debug_start_end gets constructed > directly in the instance of the top-level caller). To confirm this, I > built GDB with -fno-elide-constructors, and now I see it: > > /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:147: internal-error: ~scoped_debug_start_end: Assertion `debug_print_depth > 0' failed. > > #9 0x00005614ba5f17c3 in internal_error_loc (file=0x5614b8749960 "/home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h", line=147, fmt=0x5614b8733fa0 "%s: Assertion `%s' failed.") at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:58 > #10 0x00005614b8e1b2e5 in scoped_debug_start_end<bool&>::~scoped_debug_start_end (this=0x7ffc6c5e7b40, __in_chrg=<optimized out>) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:147 > #11 0x00005614b96dbe34 in make_scoped_debug_start_end<bool&> (pred=@0x5614baad7200: true, module=0x5614b891d840 "infrun", func=0x5614b891d800 "infrun_debug_show_threads", start_prefix=0x5614b891d7c0 "enter", end_prefix=0x5614b891d780 "exit", fmt=0x0) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:235 > > Fix this by adding an m_disabled field to scoped_debug_start_end, and > setting it in the move constructor. > > Change-Id: Ie5213269c584837f751d2d11de831f45ae4a899f > --- > gdbsupport/common-debug.h | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/gdbsupport/common-debug.h b/gdbsupport/common-debug.h > index ec36d88fdea2..33b15a005f11 100644 > --- a/gdbsupport/common-debug.h > +++ b/gdbsupport/common-debug.h > @@ -138,10 +138,25 @@ struct scoped_debug_start_end > > DISABLE_COPY_AND_ASSIGN (scoped_debug_start_end); > > - scoped_debug_start_end (scoped_debug_start_end &&other) = default; > + scoped_debug_start_end (scoped_debug_start_end &&other) > + : m_debug_enabled (other.m_debug_enabled), > + m_module (other.m_module), > + m_func (other.m_func), > + m_end_prefix (other.m_end_prefix), > + m_msg (other.m_msg), Just found this nit... not that it changes anything (because this ctor isn't called in practice), but we should std::move m_msg. I'll change it locally. Well, we could std::move all fields, but it would be a bit verbose. Simon
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: >> + scoped_debug_start_end (scoped_debug_start_end &&other) >> + : m_debug_enabled (other.m_debug_enabled), >> + m_module (other.m_module), >> + m_func (other.m_func), >> + m_end_prefix (other.m_end_prefix), >> + m_msg (other.m_msg), Simon> Just found this nit... not that it changes anything (because this ctor Simon> isn't called in practice), but we should std::move m_msg. I'll change Simon> it locally. I think it's also fine to just leave it as-is. Simon> Well, we could std::move all fields, but it would be a bit verbose. If we think we may need this kind of behavior again, one way would be a sort of "move token" object that wraps a bool, and that sets the bool on construction and clears it on move. Then scoped_debug_start_end could just use the default move constructor again, and check the token's value in its destructor. Probably overkill for just the one case. I think your patch is ok. Tom
On 1/5/23 15:05, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > >>> + scoped_debug_start_end (scoped_debug_start_end &&other) >>> + : m_debug_enabled (other.m_debug_enabled), >>> + m_module (other.m_module), >>> + m_func (other.m_func), >>> + m_end_prefix (other.m_end_prefix), >>> + m_msg (other.m_msg), > > Simon> Just found this nit... not that it changes anything (because this ctor > Simon> isn't called in practice), but we should std::move m_msg. I'll change > Simon> it locally. > > I think it's also fine to just leave it as-is. > > Simon> Well, we could std::move all fields, but it would be a bit verbose. > > If we think we may need this kind of behavior again, one way would be a > sort of "move token" object that wraps a bool, and that sets the bool on > construction and clears it on move. Then scoped_debug_start_end could > just use the default move constructor again, and check the token's value > in its destructor. Interesting, I'll keep this in mind. I think it could apply to some scoped_restore_* objects that have global side effects, that we want to be movable, because we want to be able to have functions that return them. > Probably overkill for just the one case. I think your patch is ok. Ok, thanks, will push. Simon
diff --git a/gdbsupport/common-debug.h b/gdbsupport/common-debug.h index ec36d88fdea2..33b15a005f11 100644 --- a/gdbsupport/common-debug.h +++ b/gdbsupport/common-debug.h @@ -138,10 +138,25 @@ struct scoped_debug_start_end DISABLE_COPY_AND_ASSIGN (scoped_debug_start_end); - scoped_debug_start_end (scoped_debug_start_end &&other) = default; + scoped_debug_start_end (scoped_debug_start_end &&other) + : m_debug_enabled (other.m_debug_enabled), + m_module (other.m_module), + m_func (other.m_func), + m_end_prefix (other.m_end_prefix), + m_msg (other.m_msg), + m_with_format (other.m_with_format), + m_must_decrement_print_depth (other.m_must_decrement_print_depth), + m_disabled (other.m_disabled) + { + /* Avoid the moved-from object doing the side-effects in its destructor. */ + other.m_disabled = true; + } ~scoped_debug_start_end () { + if (m_disabled) + return; + if (m_must_decrement_print_depth) { gdb_assert (debug_print_depth > 0); @@ -194,6 +209,10 @@ struct scoped_debug_start_end construction but not during destruction, or vice-versa. We want to make sure there are as many increments are there are decrements. */ bool m_must_decrement_print_depth = false; + + /* True if this object was moved from, and the destructor behavior must be + inhibited. */ + bool m_disabled = false; }; /* Implementation of is_debug_enabled when PT is an invokable type. */