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

Hi Andrew,

thanks for getting back with that.  This was also my first approach
to fix the issue.  The only limitation I see is that we have a duplicate
entry into the frame_list when we setup the return value.  When the
dtor of prev_frame is called, it is removed again.   I see this the only
side effect of the patch.

I'm personally fine with the proposed change as this is a minimal fix
for the bug.  I agree that a perfect fix would defer the frame_info_ptr
creation until the frame-id is computed, but I think this is out of
scope for this patch.

Thanks
Stephan


> -----Original Message-----
> From: Andrew Burgess <[email protected]>
> Sent: Tuesday, 14 July 2026 17:14
> To: Rohr, Stephan <[email protected]>
> Cc: [email protected]; Tom Tromey <[email protected]>
> Subject: RE: [PATCH 1/1] gdb: set the cache information in
> 'get_prev_frame_maybe_check_cycle'
> 
> "Rohr, Stephan" <[email protected]> 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 <[email protected]>
> >> Sent: Wednesday, 8 July 2026 15:59
> >> To: Rohr, Stephan <[email protected]>
> >> Cc: [email protected]; Tom Tromey <[email protected]>
> >> Subject: RE: [PATCH 1/1] gdb: set the cache information in
> >> 'get_prev_frame_maybe_check_cycle'
> >>
> >> "Rohr, Stephan" <[email protected]> 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 <[email protected]>
> >> 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 <[email protected]>
> 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 <[email protected]>
> 
> 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.*"]
>      }
>  }
> 

Intel Deutschland GmbH

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928