From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 463B13858C53 for ; Thu, 9 Feb 2023 07:40:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 463B13858C53 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de 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-out1.suse.de (Postfix) with ESMTPS id D642B2239B; Thu, 9 Feb 2023 07:40:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1675928418; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=lX8YsnZKigtwvf1s73LUY165/Gtbx2AvycWd3tlTxaY=; b=HrOuvtz8Fi9vLvDfXGCi3OUsxh3kR1wLQh1xlPb0eEUphXEMPQ+flK45YorT5hWe1TFClv FEbpsLwpQLDEWWR4NHtECuy2TLrZryNzvS+P3cGCPY/p4kv8CuyI43fkfWdFAiFiurTNJX ZLnAcU1oOtshbGT3fIOGq5X9smo1qDI= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1675928418; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=lX8YsnZKigtwvf1s73LUY165/Gtbx2AvycWd3tlTxaY=; b=9U5oaWpHhmk6PvsNc04xMqCb99l8wnWDb/WScfuIvMeHqF63R5AnxqMyGzDaX5t+TiUkLH OJdJpBvrJJ3ryJBA== 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 BDE2B1339E; Thu, 9 Feb 2023 07:40:18 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id qS9aLWKj5GP1fgAAMHmgww (envelope-from ); Thu, 09 Feb 2023 07:40:18 +0000 Message-ID: <4a9f858c-06f0-0656-29a9-3dab53bad737@suse.de> Date: Thu, 9 Feb 2023 08:40:19 +0100 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 1/2] gdb: call frame unwinders' dealloc_cache methods through destroying the frame cache To: Simon Marchi , gdb-patches@sourceware.org References: <20230130200249.131155-1-simon.marchi@efficios.com> <20230130200249.131155-2-simon.marchi@efficios.com> Content-Language: en-US Cc: "Metzger, Markus T" From: Tom de Vries In-Reply-To: <20230130200249.131155-2-simon.marchi@efficios.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.0 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 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/30/23 21:02, Simon Marchi via Gdb-patches wrote: > Currently, some frame resources are deallocated by iterating on the > frame chain (starting from the sentinel), calling dealloc_cache. The > problem is that user-created frames are not part of that chain, so we > never call dealloc_cache for them. > > I propose to make it so the dealloc_cache callbacks are called when the > frames are removed from the frame_stash hash table, by registering a > deletion function to the hash table. This happens when > frame_stash_invalidate is called by reinit_frame_cache. This way, all > frames registered in the cache will get their unwinder's dealloc_cache > callbacks called. > > Note that at the moment, the sentinel frames are not registered in the > cache, so we won't call dealloc_cache for them. However, it's just a > theoritical problem, because the sentinel frame unwinder does not > provide this callback. Also, a subsequent patch will change things so > that sentinel frames are registered to the cache. > > I moved the obstack_free / obstack_init pair below the > frame_stash_invalidate call in reinit_frame_cache, because I assumed > that some dealloc_cache would need to access some data on that obstack, > so it would be better to free it after clearing the hash table. > For me this causes: ... (gdb) PASS: gdb.btrace/record_goto.exp: instruction-history from 19 forwards record goto 27^M /data/vries/gdb/src/gdb/record-btrace.c:1654: internal-error: bfcache_new: Assertion `*slot == NULL' failed.^M A problem internal to GDB has been detected,^M further debugging may prove unreliable.^M ----- Backtrace -----^M FAIL: gdb.btrace/record_goto.exp: record goto 27 (GDB internal error) ... Note that I've been having some problems with btrace tests, possible related to cpu/kernel combination (PRs 30073 and 30075), so this may be difficult to reproduce, I'm not sure. Thanks, - Tom > Change-Id: If4f9b38266b458c4e2f7eb43e933090177c22190 > --- > gdb/frame.c | 39 ++++++++++++++++++++++++--------------- > 1 file changed, 24 insertions(+), 15 deletions(-) > > diff --git a/gdb/frame.c b/gdb/frame.c > index a08a8f47ebc4..fed961b2a8df 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -259,6 +259,22 @@ frame_addr_hash_eq (const void *a, const void *b) > return f_entry->this_id.value == f_element->this_id.value; > } > > +/* Deletion function for the frame cache hash table. */ > + > +static void > +frame_info_del (void *frame_v) > +{ > + frame_info *frame = (frame_info *) frame_v; > + > + if (frame->prologue_cache != nullptr > + && frame->unwind->dealloc_cache != nullptr) > + frame->unwind->dealloc_cache (frame, frame->prologue_cache); > + > + if (frame->base_cache != nullptr > + && frame->base->unwind->dealloc_cache != nullptr) > + frame->base->unwind->dealloc_cache (frame, frame->base_cache); > +} > + > /* Internal function to create the frame_stash hash table. 100 seems > to be a good compromise to start the hash table at. */ > > @@ -268,7 +284,7 @@ frame_stash_create (void) > frame_stash = htab_create (100, > frame_addr_hash, > frame_addr_hash_eq, > - NULL); > + frame_info_del); > } > > /* Internal function to add a frame to the frame_stash hash table. > @@ -2048,26 +2064,19 @@ reinit_frame_cache (void) > { > ++frame_cache_generation; > > - /* Tear down all frame caches. */ > - for (frame_info *fi = sentinel_frame; fi != NULL; fi = fi->prev) > - { > - if (fi->prologue_cache && fi->unwind->dealloc_cache) > - fi->unwind->dealloc_cache (fi, fi->prologue_cache); > - if (fi->base_cache && fi->base->unwind->dealloc_cache) > - fi->base->unwind->dealloc_cache (fi, fi->base_cache); > - } > - > - /* Since we can't really be sure what the first object allocated was. */ > - obstack_free (&frame_cache_obstack, 0); > - obstack_init (&frame_cache_obstack); > - > if (sentinel_frame != NULL) > annotate_frames_invalid (); > > - sentinel_frame = NULL; /* Invalidate cache */ > invalidate_selected_frame (); > + > + /* Invalidate cache. */ > + sentinel_frame = NULL; > frame_stash_invalidate (); > > + /* Since we can't really be sure what the first object allocated was. */ > + obstack_free (&frame_cache_obstack, 0); > + obstack_init (&frame_cache_obstack); > + > for (frame_info_ptr &iter : frame_info_ptr::frame_list) > iter.invalidate (); >