RE: [PATCH 1/1] gdb: set the cache information in 'get_prev_frame_maybe_check_cycle'

[Andrew Burgess <aburgess@redhat.com>]

"Rohr, Stephan" <stephan.rohr@intel.com> writes:

> Hi Andrew,
>
> Two more nits for the commit message, see my comment bellow.

Hi Stephan,

Sorry for the delay, I took some time to think about one of the issue
you raised before responding.
>
>> -----Original Message-----
>> From: Andrew Burgess <aburgess@redhat.com>
>> Sent: Wednesday, 8 July 2026 15:59
>> To: Rohr, Stephan <stephan.rohr@intel.com>
>> Cc: gdb-patches@sourceware.org; Tom Tromey <tom@tromey.com>
>> Subject: RE: [PATCH 1/1] gdb: set the cache information in
>> 'get_prev_frame_maybe_check_cycle'
>> 
>> "Rohr, Stephan" <stephan.rohr@intel.com> writes:
>> 
>> > HI Andrew,
>> >
>> > Thanks for sharing.  The patch itself looks good.
>> >
>> > I have a few comments regarding the commit message, see below.
>> 
>> Thanks Stephan.  Below is an updated patch with an improved commit
>> message.  I also tweaked some of the comments in the actual code as,
>> upon re-reading, I found some of them not ideal.
>> 
>> Let me know what you think.
>> 
>> Thanks,
>> Andrew
>> 
>> ---
>> 
>> commit cec35caa5c173de5e98a9ac6cbc0b8ee8e15d912
>> Author: Andrew Burgess <aburgess@redhat.com>
>> Date:   Thu Jun 25 14:58:50 2026 +0000
>> 
>>     gdb: set frame_info_ptr::m_cached_id during invalidation
>> 
>>     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.
>> 
>
> This is a long sentence.  Consider splitting into two to improve readability. 
>

I've reworked this, see the updated commit message below.


>>     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:
>> 
>>       - The 'up' command sets the selected frame to a frame with
>>         level > 0.
>>       - An inferior call invalidates the selected frame.
>>       - The selected frame is rebuilt following the call-stack above.
>>         The wrapping frame_info_ptr object doesn't cache the frame-id.
>>       - The 'frame' command invokes another inferior call for the pretty
>>         printer, which flushes the frame cache.
>>       - The frame_info_ptr is reinflated, e.g., to print the next
>>         argument, and this hits the assertion mentioned above.
>> 
>>     The problem is that frames don't always know their frame-id when they
>>     are placed into a frame_info_ptr, but they always do (for frames other
>>     than #0) after get_prev_frame_maybe_check_cycle has finished.  This
>>     commit defers caching the frame-id in the frame_info_ptr until the
>>     frame cache is being flushed, at which point the frame-id is known.
>> 
>
> I think we shouldn't focus on 'get_prev_frame_maybe_check_cycle' solely, though
> I didn't find any other location where the described behaviour could reproduce
> (which doesn't mean it doesn't exist).

I think in GDB right now get_prev_frame_maybe_check_cycle is the only
place this bug exists.

There are 3 places where new frame_info objects are created:
create_sentinel_frame, create_new_frame, and get_prev_frame_raw.

In the first two of these the frame_info is assigned an ID before being
placed into the frame_info_ptr, so these are not problems.

Only in get_prev_frame_raw is the frame_info placed into a
frame_info_ptr before having an ID assigned, and that is only called
from get_prev_frame_maybe_check_cycle.

As we discussed in another thread, an ideal solution would be to have
get_prev_frame_raw not create the frame_info_ptr at all, and defer this
until get_prev_frame_maybe_check_cycle has finished, but this would
require changing the frame sniffer API to not expect a frame_info_ptr,
which seems like a bigger change than I'd like to make right now.

But as I wrote the above I'm finding it harder to justify the churn of
moving the frame_id caching into the frame_info_ptr::invalidate method.

So, sorry to pivot again, but how about the patch below?  It's far
simpler than the original suggestion and is targets just
get_prev_frame_maybe_check_cycle, which is where the broken
frame_info_ptr objects always come from.

Let me know what you think.

Thanks,
Andrew

--

commit b6dba04e0e21f1a83b43ca616ae5fcce1a66eb9b
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Jun 25 14:58:50 2026 +0000

    gdb: recreate the frame_info_ptr in get_prev_frame_maybe_check_cycle
    
    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.
    
    The function get_prev_frame_maybe_check_cycle calls get_prev_frame_raw
    to create the previous frame, placing the result into a frame_info_ptr
    PREV_FRAME.  For frames other than frame 0, compute_frame_id is then
    called computing the frame-id.  However, the call to compute_frame_id
    only updates the frame_info object itself, the frame_info_ptr
    PREV_FRAME is not updated with the new frame-id.
    
    What this means is that in get_prev_frame_maybe_check_cycle, the
    PREV_FRAME local has no cached frame-id.
    
    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
    
    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:
    
      - The 'up' command sets the selected frame to a frame with
        level > 0.
      - An inferior call invalidates the selected frame.
      - The selected frame is rebuilt following the call-stack above.
        The wrapping frame_info_ptr object doesn't cache the frame-id.
      - The 'frame' command invokes another inferior call for the pretty
        printer, which flushes the frame cache.
      - The frame_info_ptr is reinflated, e.g., to print the next
        argument, and this hits the assertion mentioned above.
    
    There are only 3 places in GDB where new frame_info objects are
    created: create_sentinel_frame, create_new_frame, and
    get_prev_frame_raw.  Of these, the first two always calculate the
    frame_id before placing the frame_info object into a frame_info_ptr.
    
    Only get_prev_frame_raw, which is only called from
    get_prev_frame_maybe_check_cycle, creates the frame_info_ptr before
    the frame_id is calculated.
    
    There are two places where PREV_FRAME is returned from
    get_prev_frame_maybe_check_cycle.  The first is only for frame #0.
    The frame_info_ptr::reinflate method doesn't need a frame_id for
    frame #0, so the first return is not a problem.
    
    The second return from get_prev_frame_maybe_check_cycle is done after
    the frame_id has been calculated, and it is here that the problem can
    be fixed.  If we create a new frame_info_ptr to replace PREV_FRAME
    then this new frame_info_ptr will have a cached frame_id and the
    problem described above will no longer occur.
    
    Co-Authored-By: Rohr, Stephan <stephan.rohr@intel.com>

diff --git a/gdb/frame.c b/gdb/frame.c
index cefdde5ed1e..b91e18fad99 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2332,7 +2332,16 @@ get_prev_frame_maybe_check_cycle (const frame_info_ptr &this_frame)
       throw;
     }
 
-  return prev_frame;
+  /* When PREV_FRAME was initially created it had no cached frame_id as the
+     frame_id had not yet been computed.  Without a frame_id however
+     PREV_FRAME will not be able to reinflate.  Recreate the frame_info_ptr
+     now that the frame_id is known, this new frame_info_ptr will have a
+     cached frame_id.
+
+     You might wonder about the earlier return of PREV_FRAME within the
+     function.  That is fine as reinflating a frame_info_ptr at level 0
+     doesn't require a cached frame_id.  */
+  return frame_info_ptr (prev_frame.get ());
 }
 
 /* Helper function for get_prev_frame_always, this is called inside a
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.*"]
     }
 }