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