[1/2] gdb: call frame unwinders' dealloc_cache methods through destroying the frame cache
Message ID | 20230130200249.131155-2-simon.marchi@efficios.com |
---|---|
State | Committed |
Commit | 6d3717d4c412db52b33fa53f0ea9fc52f3d0aadc |
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 48F7F3858291 for <patchwork@sourceware.org>; Mon, 30 Jan 2023 20:03:19 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 48F7F3858291 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1675108999; bh=lSx3ggRkmnvkETYOvHODRHqM4JIWj8/3j5v4gGz3r3E=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=cC8UonfUZxbkh50d3aw74SMjELNIwty7mhWVHCQTQHsjiNnFdCHwW1gR7OReS0kmd 7vEuBEikAj5lq5SV45MF+ICTuBPpPDqrWaWsN+u0SjAspot2n4T96fvcqGsGwpDr0A 94jmNUEOQbMkRsaAeckWuTBBwl/WxspPrA6c4db0= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id 218D53858291 for <gdb-patches@sourceware.org>; Mon, 30 Jan 2023 20:02:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 218D53858291 X-ASG-Debug-ID: 1675108970-0c856e762b85c7f0001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id 5W6YMUmgDDlxl44D (version=TLSv1 cipher=AES128-SHA bits=128 verify=NO); Mon, 30 Jan 2023 15:02:50 -0500 (EST) X-Barracuda-Envelope-From: simon.marchi@efficios.com X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from smarchi-efficios.internal.efficios.com (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) by smtp.ebox.ca (Postfix) with ESMTP id 6B031441B21; Mon, 30 Jan 2023 15:02:50 -0500 (EST) X-Barracuda-RBL-IP: 192.222.180.24 X-Barracuda-Effective-Source-IP: 192-222-180-24.qc.cable.ebox.net[192.222.180.24] X-Barracuda-Apparent-Source-IP: 192.222.180.24 To: gdb-patches@sourceware.org Cc: Simon Marchi <simon.marchi@efficios.com> Subject: [PATCH 1/2] gdb: call frame unwinders' dealloc_cache methods through destroying the frame cache Date: Mon, 30 Jan 2023 15:02:48 -0500 X-ASG-Orig-Subj: [PATCH 1/2] gdb: call frame unwinders' dealloc_cache methods through destroying the frame cache Message-Id: <20230130200249.131155-2-simon.marchi@efficios.com> X-Mailer: git-send-email 2.39.1 In-Reply-To: <20230130200249.131155-1-simon.marchi@efficios.com> References: <20230130200249.131155-1-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1675108970 X-Barracuda-Encrypted: AES128-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Barracuda-BRTS-Status: 1 X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 3676 X-Barracuda-Spam-Score: 0.00 X-Barracuda-Spam-Status: No, SCORE=0.00 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=5.0 tests= X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.104094 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- X-Spam-Status: No, score=-3498.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_SOFTFAIL, 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 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@efficios.com> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
Fix gdb.base/frame-view.exp on AArch64
|
|
Commit Message
Simon Marchi
Jan. 30, 2023, 8:02 p.m. UTC
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. Change-Id: If4f9b38266b458c4e2f7eb43e933090177c22190 --- gdb/frame.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)
Comments
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 (); >
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 (); >
On 2/9/23 07:42, Tom de Vries via Gdb-patches wrote: > 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 for reporting. I managed to reproduce on gcc13 (I only have computers with AMD cpus at the moment, so I can't run the btrace tests). I sent a patch, the explications of the cause and proposed solution are there: https://inbox.sourceware.org/gdb-patches/20230209195037.100368-1-simon.marchi@efficios.com/T/#u Simon
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 ();