From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id VCdAIJ0jTWqOMSsAWB0awg (envelope-from ) for ; Tue, 07 Jul 2026 12:04:45 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=i5ICalNt; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 6D9C81E024; Tue, 07 Jul 2026 12:04:45 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [IPv6:2620:52:6:3111::32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 6D2C21E024 for ; Tue, 07 Jul 2026 12:04:44 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id EB7114BA23E7 for ; Tue, 7 Jul 2026 16:04:43 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org EB7114BA23E7 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=i5ICalNt Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id D74354BA5436 for ; Tue, 7 Jul 2026 16:04:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D74354BA5436 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org D74354BA5436 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1783440256; cv=none; b=WlZd83Q57UFtRTEH6aN8I7qYja7gVbyfaZ3voLeg53NvrcbMK41d+a92JxAqql0YsntMfIe4NBUWSH/XCMOTbE+5VY/dJOP9tNYGfayEeu3PM+bLbkU+e7HomyqXmCU/cZG3TrN7E46BZ+4Ebap017fUbe49xLyyOjgFA91ulFQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1783440256; c=relaxed/simple; bh=ZoCgjYvQz14H0AvDY7W6PalqQ3wOHaMPUvPWlU65EYc=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=tqRdoQsMCdhKX96c1x4eEgRfF7q6TYB51MTBwk1wNaDoN2jyOLyKWvv3uqmUtXxPm5yVBfKGEHIEgUBQi3wIQ4eWZbjnjZDnNAKSUP744aLMkmLrm8z7IcavSobq2YuUXdM1m5+LzkFFzrt94ioTYggW5/OQMxnAxblD8H5v4dg= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=i5ICalNt DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D74354BA5436 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1783440255; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=/ha7AuCrmzvN7FOwAzxROcL4rEFotw/dQ14fzi3LneY=; b=i5ICalNtcYIaZhQN8c0wP4QLLqixsDO4yJARJFs5KoBuCR7qy2bIbE7E/U07KdqA351Haq QRsBmQ+OuTB3zjxVqHvl/RwHJPzN98aZShFLCO+IrC59ZThRU3nR4AVK5w4CK6T0bMXdyQ FhoxXKBM0HekbM9rKVtovGh/YJ0bfrs= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-157-V1Huf_XmMrO6eIleIa6V9w-1; Tue, 07 Jul 2026 12:04:14 -0400 X-MC-Unique: V1Huf_XmMrO6eIleIa6V9w-1 X-Mimecast-MFC-AGG-ID: V1Huf_XmMrO6eIleIa6V9w_1783440253 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-493dc8408fdso15103875e9.1 for ; Tue, 07 Jul 2026 09:04:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783440253; x=1784045053; h=content-type:mime-version:message-id:date:references:in-reply-to :subject:cc:to:from:x-gm-gg:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to:content-type; bh=/ha7AuCrmzvN7FOwAzxROcL4rEFotw/dQ14fzi3LneY=; b=FH+UTRYJ7OjJmyk0yk4Pm4k6KsJzCYpJrdaN/7QYBaX22CXlaDBz6Zzxr5XYt7jawD JBtmqSPN8uWRHZmzRHW4oXQPiuCmlodPbKq95E/CUztqjG/OzODm4hKCYjI5uQRhAuFn AjQh7DAvGxfmVsNhH5U0cKFjRx1UeA1cTqbch1SuYuD5xI9vVS92Ym6FS+JrxS1FYLes Lg/pViNDnpHThkV9TAxQs/ixS5KmaPYZfRaFkYd2PlqYnzGixwcNiVR20WBO7Jc56gh1 KUCkSWIkOkKrZGMCt6a8TDWT1bToYU6CGH8hyDo+x4l4g8340NVCvFeYPauJ96t1iLUE 55sA== X-Gm-Message-State: AOJu0YwFXhND4OrIXxag+LJm5BlV1JCxscl//dG9ITE2soCYgm0hYPaI o9MojJmdmX6djGjdcWYVlorX2kySPckwqvlIEiF5mVb3ZghYrk5Y+rXZFa+1t2GDlOskBDfuVcR ED2gNbJDlTHHaM5XJfWqDN/6HYSgXD2eHERaHRrxTNBWdvmnDKdoUqzL1AlSn2riAwa8Pxjw= X-Gm-Gg: AfdE7cle1SRJsfMBAslXVQRzCqKA6WJ40fgrTOSIRAbByQsFr0SyBagvkN6iPJv1L4q xVRawMmZ5Yy5mOgBASzs6KSm/FfXdf7NQDMZFWH+jXCSdNq2s24LoA7okuJZ96+SRHu3TzKPrnM 6yZhlhmzT5Aee67z/IIXc7yR4k8vN+NgU82LAB2wYcmMrehIgbvK1uf/Xi2EXyx/5Cn2ybGthG1 DvqOiEx0AqFyB15pZlDFS4ppnpDmp3oQ4viMIOqfzP7GaPLpG1rHWR7APcE94VxullB4CJNcnnw P+c9EVan0PYxSB4106UN80nb8FCJxZD/cZrNZFYY1ooYJgdZ791F0lYia6WEqoQW5GieoAmTsKQ UWUcwMjg= X-Received: by 2002:a05:600c:8189:b0:490:469c:556b with SMTP id 5b1f17b1804b1-493df06afaemr55148615e9.12.1783440252323; Tue, 07 Jul 2026 09:04:12 -0700 (PDT) X-Received: by 2002:a05:600c:8189:b0:490:469c:556b with SMTP id 5b1f17b1804b1-493df06afaemr55148125e9.12.1783440251779; Tue, 07 Jul 2026 09:04:11 -0700 (PDT) Received: from localhost ([31.111.209.233]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-493e0f40d50sm74461375e9.5.2026.07.07.09.04.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Jul 2026 09:04:10 -0700 (PDT) From: Andrew Burgess To: "Rohr, Stephan" Cc: "gdb-patches@sourceware.org" , Tom Tromey Subject: RE: [PATCH 1/1] gdb: set the cache information in 'get_prev_frame_maybe_check_cycle' In-Reply-To: References: <20260625145850.3104079-1-stephan.rohr@intel.com> <20260625145850.3104079-2-stephan.rohr@intel.com> <87se69wdbt.fsf@tromey.com> <87zf04tawh.fsf@redhat.com> Date: Tue, 07 Jul 2026 17:04:09 +0100 Message-ID: <87ldbmu6py.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: nJ62gvCMZ_bFsCrpgid9NcfyBtG3GSf8y1nelWh9P_g_1783440253 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org "Rohr, Stephan" writes: > Hi Andrew, > > thanks for sharing this! > > One option that could cause a frame_info_ptr w/o a set ID is > the one I found in 'get_prev_frame_maybe_check_cycle ()'. > > It is not obvious why this causes an assertion when reinflating the > pointer later. My explanation how to reproduce the bug is: > > 1. The 'up' command updates 'selected_frame' to the frame at > level 1. > > 2. The inferior call 'p f()' triggers a re-init of the frame cache. The > selected_frame pointer is nulled, but the cached id is still valid. > > 3. The 'frame' command find 'selected_frame' is null, so it rebuilds > by calling get_selected_frame --> lookup_selected_frame > --> get_prev_frame --> get_prev_frame_maybe_check_cycle. > A new frame_info_ptr is setup but its ID is only computed a few lines > later. select_frame () then copies this uncached frame_info_ptr into > selected frame. > > 4. The pretty-printer triggers another infcall: > > 'MytypePrinter.to_string()' --> gdb.parse_and_eval("f()") > --> call_function_by_hand () > > This invalidates the pointer while printing the 'mt' argument. > When printing the next argument, the pointer is reinflated via > read_frame_arg --> ... --> get_frame_pc --> reinflate. Since level > 0 > and m_ptr == nullptr, reinflation is done by the cached id. This is not > set and causes the assertion to fire. > > Does this help? > > In general, I'm in favour of your patch as it is more generic and fixes all > issues where we have a frame_info_ptr setup w/o an ID. > > I have a few comments, see below. > > Thanks > Stephan > >> -----Original Message----- >> From: Andrew Burgess >> Sent: Monday, 6 July 2026 17:07 >> To: Rohr, Stephan ; Tom Tromey >> >> Cc: gdb-patches@sourceware.org >> Subject: RE: [PATCH 1/1] gdb: set the cache information in >> 'get_prev_frame_maybe_check_cycle' >> >> "Rohr, Stephan" writes: >> >> > Hi Tom, >> > >> > thank you for your feedback! I will fix the typos in v2 of the patch. >> > >> > I'll try to explain the problem in more detail now: >> > >> > 1. If you run an inferior call after the up command invalidates >> > the selected frame. >> > 2. The selected frame is rebuild following the chain >> > >> > get_selected_frame () -> lookup_selected_frame () >> > -> get_prev_frame_always_1 () >> > -> get_prev_frame_maybe_check_cycle () >> > >> > 3. At this point, the frame_info_ptr is constructed before the >> > frame-id is computed. The cache Information is not updated as >> > 'compute_frame_id' only updates the frame id of the internal >> > 'frame_info' pointer. >> > >> > 4. If we now run the "frame" command in combination with a >> > pretty-printer, this invokes another infcall to evaluate the >> > pretty-printer. >> > >> > 5. This clears the 'm_ptr' member of the frame_info_ptr. Since the cache >> > id is not set, a subsequent reinflate fails. >> > >> > A very simple fix for this could be: >> > >> > @@ -2332,7 +2361,7 @@ get_prev_frame_maybe_check_cycle (const >> frame_info_ptr &this_frame) >> > throw; >> > } >> > >> > - return prev_frame; >> > + return frame_info_ptr (prev_frame.get ()); >> > } >> > >> > It constructs a new frame_info_ptr using the updated raw pointer >> > of 'prev_frame'. This is somewhat redundant as another frame_info_ptr >> > is added to the frame list in the frame_info_ptr ctor. >> > >> > The frame list entry from >> > >> > frame_info_ptr prev_frame = get_prev_frame_raw (this_frame); >> > >> > is removed again by the frame_info_ptr's dtor. >> > >> > This would add some overhead. >> > >> > I didn't find a better solution. We could modify 'get_prev_frame_raw' to >> > return a raw pointer instead (it is the only location where is this called at >> > all). But we'd need a temporary copy of the frame_info_ptr anyways, >> > either to pass a frame_info_ptr to 'compute_frame_id' or inside of >> > 'compute_frame_id' (to forward the frame id to >> > 'frame_unwind_find_by_frame'). Changing these functions to >> > accept a raw frame_info pointer is not desired in my point of view. >> > >> > I appreciate your feedback. >> > >> >> Hi Stephan, >> >> I ran into a similar problem myself recently, and had a different fix >> queued up, but it was part of a larger change. I've pulled it out and >> the patch is below, along with your test change. The test still passes. >> >> There's no commit message yet, I still need to think about exactly >> what's going on in this case a bit more, it's still not clear to me how >> (or where) the frame_info_ptr without the frame-id is actually created, >> as I thought frames (after 0) always got a frame-id before they were >> placed into a frame_info_ptr. >> >> I'm sharing this now just so you can take a look at the change and let >> me know what you think, I'll dig into this a little more and write up a >> commit message tomorrow, but I'd like to hear what you think of this >> approach. >> >> Thanks, >> Andrew >> >> --- >> >> diff --git a/gdb/frame.c b/gdb/frame.c >> index cefdde5ed1e..7a0f313f4db 100644 >> --- a/gdb/frame.c >> +++ b/gdb/frame.c >> @@ -2190,15 +2190,15 @@ reinit_frame_cache (void) >> sentinel_frame = nullptr; >> } >> >> + for (frame_info_ptr &iter : frame_info_ptr::frame_list) >> + iter.invalidate (); >> + > > I see why we need this, but it's worth a comment that the invalidate > loop must run before invalidating the frame stash. Added in my next version. > >> 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 (); >> - >> frame_debug_printf ("generation=%d", frame_cache_generation); >> } >> >> @@ -3435,10 +3435,21 @@ frame_info_ptr::frame_info_ptr (struct >> frame_info *ptr) >> if (m_ptr == nullptr) >> return; >> >> - m_cached_level = ptr->level; >> + m_cached_level = m_ptr->level; > > Do we need this? At this point, we should have m_ptr == ptr? I'll revert this for the final patch. > >> +} >> + >> +void >> +frame_info_ptr::invalidate () >> +{ >> + if (m_ptr == nullptr) >> + return; >> + >> + gdb_assert (m_cached_level == m_ptr->level); >> >> if (m_cached_level != 0 || m_ptr->this_id.value.user_created_p) >> m_cached_id = m_ptr->this_id.value; > > I was wondering if we should add another guard: > > if ((m_cached_id == null_frame_id) > && (m_cached_level != 0 || m_ptr->this_id.value.user_created_p)) > > Or is this implicitly guarded by the 'm_cached_level != 0' check? Comparing to null_frame_id always returns false, see frame_id::operator== for details. If we _could_ compare to null_frame_id then we could add this assert: gdb_assert (m_cached_id == null_frame_id || m_cached_id == m_ptr->this_id.value); This means that, if a frame_info_ptr is invalidated multiple times, then the m_cached_id will be updated multiple times, but it should never change. But, as we cannot compare to null_frame_id, I'm not sure how we'd write that assert. Given that we check the frame-id matches when we reinflate though, I'm not sure the above assert adds much. Your proposal (if we could compare to null_frame_id) would prevent the repeated updates, but isn't needed to prevent any invalid behaviour, for the same reason, the frame-id should never be different. The updated patch is below, let me know what you think. Thanks, Andrew --- commit d13aa44f8ac57e52324da409145f73a0e3ca19a2 Author: Andrew Burgess Date: Thu Jun 25 14:58:50 2026 +0000 gdb: set frame_info_ptr::m_cached_id in the destructor Currently frame_info_ptr caches the frame_id at construction time, see frame_info_ptr::frame_info_ptr in frame.c. The problem with this is that a frame's frame-id might not be known at this point. Consider get_prev_frame_maybe_check_cycle, this calls get_prev_frame_raw to create the previous frame, placing the result into a frame_info_ptr PREV_FRAME. Then (for frames other than frame 0) compute_frame_id is called, however, this only computes the frame_id for the frame_info object pointed to by the frame_info_ptr, the cached frame_id within the frame_info_ptr itself is not updated. What this means is that in get_prev_frame_maybe_check_cycle, the PREV_FRAME local has no cached frame-id. If we consider the call stack: get_selected_frame lookup_selected_frame frame_find_by_id get_prev_frame get_prev_frame_always get_prev_frame_always_1 get_prev_frame_maybe_check_cycle Then what we see is that the frame_info_ptr created in get_prev_frame_maybe_check_cycle, which lacks a cached frame_id, can be passed all the way back to lookup_selected_frame, where it will be stored in the SELECTED_FRAME global by a call to select_frame. The outer get_selected_frame call (in the above backtrace) will then return the SELECTED_FRAME global, which lacks a cached frame-id. If GDB ever tries to reinflate the SELECTED_FRAME frame_info_ptr (or a copy of it), then we will trigger the assert: `gdb_assert (frame_id_p (m_cached_id));` which can be found in `frame_info_ptr::reinflate` in frame.c. An example of how this can be triggered is included in the updated test case, frame #1 is selected and the frame is printed. The pretty printer performs an inferior call which invalidates the frame cache, the assertion then triggers when trying to reinflate the selected frame frame_info_ptr. The problem is that frame's don't always know their frame-id when they are placed into a frame_info_ptr, but they always do (for frame other than #0) after get_prev_frame_maybe_check_cycle has finished. We could try to have get_prev_frame_maybe_check_cycle or compute_frame_id push the computed frame-id into the frame_info_ptr, or we can just defer caching the frame-id until we know we might need it, i.e. when the frame cache is being flushed. This second approach is actually nice in that it defers the work until we know we need it, and frame_info_ptr objects that are created and destroyed without the frame cache ever being flushed no longer need to cache the frame-id. This isn't going to give any noticable performance improvement, but still, it feels nice. The changes in this commit then are: 1. In reinit_frame_cache we call frame_info_ptr::invalidate before deleting all the frame_info objects (by clearing the obstacks), this allows us to copy the frame_id from these objects. 2. In frame_info_ptr::frame_info_ptr we still need to record every frame_info_ptr in the global list, and for now at least, we still cache the frame level, this is needed for frame_info_ptr::is_null, which checks the cached level. 3. In frame_info::invalidate, we can assert that the cached level matches the stored frame's level, this should never change, and it is here that we now cache the frame-id. Co-Authored-By: Rohr, Stephan diff --git a/gdb/frame.c b/gdb/frame.c index cefdde5ed1e..e7974060cb8 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -2190,15 +2190,18 @@ reinit_frame_cache (void) sentinel_frame = nullptr; } + /* Invalidation copies the frame-id from the managed frame_info object + into the frame_info_ptr, so this must run before the frame_info + objects are invalidated. */ + for (frame_info_ptr &iter : frame_info_ptr::frame_list) + iter.invalidate (); + 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 (); - frame_debug_printf ("generation=%d", frame_cache_generation); } @@ -3436,9 +3439,23 @@ frame_info_ptr::frame_info_ptr (struct frame_info *ptr) return; m_cached_level = ptr->level; +} +void +frame_info_ptr::invalidate () +{ + if (m_ptr == nullptr) + return; + + gdb_assert (m_cached_level == m_ptr->level); + + /* If a frame_info_ptr is invalidated multiple times then we will end up + updating m_cached_id multiple times. This should be harmless as the + underlying frame_id should never change. */ if (m_cached_level != 0 || m_ptr->this_id.value.user_created_p) m_cached_id = m_ptr->this_id.value; + + m_ptr = nullptr; } /* See frame-info-ptr.h. */ diff --git a/gdb/frame.h b/gdb/frame.h index f6553fb7b6d..b386303d895 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -327,10 +327,7 @@ class frame_info_ptr : public intrusive_list_node } /* Invalidate this pointer. */ - void invalidate () - { - m_ptr = nullptr; - } + void invalidate (); private: /* We sometimes need to construct frame_info_ptr objects around the diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp index 52162fc9952..a2a29c4d0f8 100644 --- a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp @@ -108,6 +108,8 @@ with_test_prefix "frame movement down" { with_test_prefix "frame movement up" { if { [start_test "TAG: final frame"] == 0 } { gdb_test "up" [multi_line "#1 .*in g \\(mt=mytype is .*\\, depth=1\\).*" ".*first frame.*"] + gdb_test "p f ()" " = 2" + gdb_test "frame" [multi_line "#1 .*in g \\(mt=mytype is .*\\, depth=1\\).*" ".*first frame.*"] } }