From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id 01A9E3858C53 for ; Fri, 5 Aug 2022 15:03:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 01A9E3858C53 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id D2D8D20DDD; Fri, 5 Aug 2022 15:03:25 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id AC99913A9C; Fri, 5 Aug 2022 15:03:25 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id GpiqKD0x7WLTVAAAMHmgww (envelope-from ); Fri, 05 Aug 2022 15:03:25 +0000 Message-ID: <8f62af78-0305-65e3-b728-282c310a7cae@suse.de> Date: Fri, 5 Aug 2022 17:03:25 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH 1/3] gdb/varobj: Do not invalidate locals in varobj_invalidate_iter Content-Language: en-US To: Lancelot SIX , gdb-patches@sourceware.org Cc: lsix@lancelotsix.com References: <20220804130231.2126565-1-lancelot.six@amd.com> <20220804130231.2126565-2-lancelot.six@amd.com> From: Tom de Vries In-Reply-To: <20220804130231.2126565-2-lancelot.six@amd.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_NONE, 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: Fri, 05 Aug 2022 15:03:37 -0000 On 8/4/22 15:02, Lancelot SIX wrote: > The varobj_invalidate_iter function has logic to invalidate any local > varobj it can find. However since bc20e562ec0 "gdb/varobj: Fix use > after free in varobj" all varobj containing references to an objfile are > cleared when the objfile goes out of scope. This means that at this > point any local varobj seen by varobj_invalidate_iter either has > already been invalidated by varobj_invalidate_if_uses_objfile or only > contains valid references and there is no reason to invalidate it. > > This patch proposes to remove this un-necessary invalidation and adds a un-necessary - > unnecessary > testcase which exercises a scenario where a local varobj can legitimately > survive a call to varobj_invalidate_iter. > > At this point the varobj_invalidate and varobj_invalidate seem misnamed Missing _iter on the latter ? > since they deal with re-creating invalid objects and do not do > invalidation, but this will be fixed in a following patch. > > Tested on x86_64-linux. > --- > .../gdb.mi/mi-var-invalidate-shlib.exp | 40 +++++++++++++++++-- > gdb/varobj.c | 2 - > 2 files changed, 36 insertions(+), 6 deletions(-) > > diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp > index 87d1d4a6a9d..9ba5af0c028 100644 > --- a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp > +++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp > @@ -40,10 +40,6 @@ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $opt > } > > proc do_test { separate_debuginfo } { > - if { $separate_debuginfo } { > - gdb_gnu_strip_debug $::shlib_path > - } > - > if { [mi_clean_restart] } { > unsupported "failed to start GDB" > return > @@ -132,6 +128,42 @@ proc do_test { separate_debuginfo } { > } > } > > +proc_with_prefix local_not_invalidated { separate_debuginfo } { > + if { [mi_clean_restart] } { > + unsupported "failed to start GDB" > + return > + } > + > + # Start the process once and create varobjs referencing the loaded objfiles. > + with_test_prefix "setup" { > + mi_load_shlibs $::shlib_path > + if { $separate_debuginfo } { > + mi_load_shlibs ${::shlib_path}.debug > + } > + > + mi_gdb_reinitialize_dir $::srcdir/$::subdir > + mi_gdb_load $::binfile > + > + mi_runto foo -pending > + mi_next "next" > + mi_create_varobj local_var local_var "create local varobj" > + } > + > + # A this point we are stoped in the shared library. If we reload symbols stoped -> stopped. Otherwise, LGTM. Thanks, - Tom > + # for the main binary, symbols for the shared library remain valid. > + # A varobj tracking variables in the scope of the shared library only > + # should not be invalidated. > + mi_gdb_load ${::binfile} > + mi_gdb_test "-var-update local_var" \ > + "\\^done,changelist=\\\[\\\]" \ > + "local_var preserved" > +} > + > foreach_with_prefix separate_debuginfo {0 1} { > + if { $separate_debuginfo } { > + gdb_gnu_strip_debug $::shlib_path > + } > + > do_test $separate_debuginfo > + local_not_invalidated $separate_debuginfo > } > diff --git a/gdb/varobj.c b/gdb/varobj.c > index 0683af1991e..a142bb02e03 100644 > --- a/gdb/varobj.c > +++ b/gdb/varobj.c > @@ -2387,8 +2387,6 @@ varobj_invalidate_iter (struct varobj *var) > var->root->is_valid = false; > } > } > - else /* locals must be invalidated. */ > - var->root->is_valid = false; > } > > /* Invalidate the varobjs that are tied to locals and re-create the ones that