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,
>
> 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
    
    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 <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.*"]
     }
 }