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

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

HI Andrew,

Thanks for sharing.  The patch itself looks good.

I have a few comments regarding the commit message, see below.

Thanks
Stephan

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Tuesday, 7 July 2026 18:04
> 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 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 <aburgess@redhat.com>
> >> Sent: Monday, 6 July 2026 17:07
> >> To: Rohr, Stephan <stephan.rohr@intel.com>; Tom Tromey
> >> <tom@tromey.com>
> >> Cc: gdb-patches@sourceware.org
> >> 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 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 <aburgess@redhat.com>
> Date:   Thu Jun 25 14:58:50 2026 +0000
> 
>     gdb: set frame_info_ptr::m_cached_id in the destructor

I think this is misleading as it's not updated in the dtor but when the
frame_info_ptr is invalidated.

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

We can put a bit more detail here, e.g.:

  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 rebuild following the call-stack above.
      The wrapping frame_info_ptr object doesn't cache the frame-id.
    - If the frame is printed with the 'frame' command, this invokes another
      inferior call for the pretty printer, which flushes the frame cache.
    - If the frame_info_ptr is reinflated, this hits the assertion mentioned above, e.g.,
      to print the next argument.

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

I think we don't need to explain the two options here.  Maybe add one sentence
summarizing the change, e.g.:

  Update the cache-id of the frame_info_ptr before removing flushing the frame-cache.

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

I think we don't need items 1. to 3. as part of the commit message.

>     Co-Authored-By: Rohr, Stephan <stephan.rohr@intel.com>
> 
> 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<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