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

["Rohr, Stephan" <stephan.rohr@intel.com>]

Hi Andrew,

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

Thanks

Stephan 

> -----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. 

>     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).

Maybe generalize a bit more, e.g.:

     The problem is that a frame_info_ptr relies on a computed frame-id upon
     construction time to cache the frame-id.  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'm not sure if 'defer' is the correct wording as the frame_info_ptr ctor still
sets the frame-id.  Maybe change to 'updates the frame-id'.

>     Co-Authored-By: Rohr, Stephan <stephan.rohr@intel.com>
> 
> diff --git a/gdb/frame.c b/gdb/frame.c
> index cefdde5ed1e..a0a982dc866 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 freed (via obstack_free).  */
> +  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,24 @@ 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 (with a reinflate
> +     call between) 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<frame_info_ptr>
>    }
> 
>    /* 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.*"]
>      }
>  }
> 

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