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 C10623858C50 for ; Thu, 9 Feb 2023 12:42:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C10623858C50 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-out2.suse.de (Postfix) with ESMTPS id 073445CDA3; Thu, 9 Feb 2023 12:42:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1675946545; 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=l6i7fehRetTHtplMPRsUpAHw8hfqEDpzr7gbIA6AMOY=; b=F3V60fWhw0ABPllsdrep9HpXmbg/lQjr8bAR8+R5gqVSkBHmWZKMY1ThKSE6zdnJtosN/n PCVXiIgE2YFsoq+1cSJLpQBWB9sxav/87OcSZYwJm8XatbcpQ9Wyt9uAxrMWfflgvqquHY JRxocnNAFIJnstXnfkS67yRP3CLcK88= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1675946545; 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=l6i7fehRetTHtplMPRsUpAHw8hfqEDpzr7gbIA6AMOY=; b=qP9FovvDrYRjKlLgDNyu0ipXgPSVENdlC8dCHOfwMn52R5DrDg6QzLoFiu+/ZXEahbzMGd mi3DexE0I2KX5mCg== 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 E16501339E; Thu, 9 Feb 2023 12:42:24 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id lmfRNTDq5GNLXgAAMHmgww (envelope-from ); Thu, 09 Feb 2023 12:42:24 +0000 Message-ID: Date: Thu, 9 Feb 2023 13:42:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH 1/2] gdb: call frame unwinders' dealloc_cache methods through destroying the frame cache From: Tom de Vries To: Simon Marchi , gdb-patches@sourceware.org Cc: "Metzger, Markus T" References: <20230130200249.131155-1-simon.marchi@efficios.com> <20230130200249.131155-2-simon.marchi@efficios.com> <4a9f858c-06f0-0656-29a9-3dab53bad737@suse.de> Content-Language: en-US In-Reply-To: <4a9f858c-06f0-0656-29a9-3dab53bad737@suse.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,BODY_8BITS,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 2/9/23 08:40, Tom de Vries wrote: > 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. > I also managed to reproduce this on openSUSE Tumbleweed, which doesn't show the problems with btrace tests, so I'm hoping this is easy to reproduce. Thanks, - Tom > 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 (); >