From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 0F9583858D35 for ; Wed, 4 Jan 2023 22:01:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0F9583858D35 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca 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 304M1G7P028530 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 4 Jan 2023 17:01:21 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 304M1G7P028530 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1672869681; bh=CVULBoQtEJlaZgw3LW/N8BWjVxlYZ8jxv9lEkSnR+VI=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Xyw5T8OfTwk0ms55dPp0SD5lyAWLDc9UFEcnkB0+PYB+I3avOnGg37KhlMCN1pXX9 xtVaGO9OoSRLlqoIiD5n/m0aQISKFX0ZLSdTRttjKpHewwP5dlDWuE5eZTZO5NdFyY 0CtQU4BqQC+s0TnTUZLvmNCJsD3URPGXaUHba/u8= Received: from [10.0.0.11] (unknown [217.28.27.60]) (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 B268E1E112; Wed, 4 Jan 2023 17:01:16 -0500 (EST) Message-ID: Date: Wed, 4 Jan 2023 17:01:16 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH] gdbsupport: fix scoped_debug_start_end's move constructor Content-Language: en-US To: gdb-patches@sourceware.org Cc: Andrew Burgess References: <20230104212242.545914-1-simon.marchi@polymtl.ca> From: Simon Marchi In-Reply-To: <20230104212242.545914-1-simon.marchi@polymtl.ca> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 4 Jan 2023 22:01:16 +0000 X-Spam-Status: No, score=-3038.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,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 List-Id: 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 > static inline scoped_debug_start_end 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 (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::~scoped_debug_start_end (this=0x7ffc6c5e7b40, __in_chrg=) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:147 > #11 0x00005614b96dbe34 in make_scoped_debug_start_end (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