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

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

[Stephan Rohr]

From: "Rohr, Stephan" <stephan.rohr@intel.com>

Hello all,

I encountered an issue that GDB tries to reinflate a frame whose ID is
not cached yet.  I could only reproduce by invoking an inferior call
in conjunction with a Python pretty printer; more details in the commit
message.

Regression tested on Ubuntu 24 using the unix target board file.

I appreciate your feedback.

Thanks
Stephan

Rohr, Stephan (1):
  gdb: set the cache information in 'get_prev_frame_maybe_check_cycle'

 gdb/frame.c                                    | 18 +++++++++++++++++-
 gdb/frame.h                                    |  4 ++++
 .../gdb.python/pretty-print-call-by-hand.exp   |  2 ++
 3 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.43.0

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

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

[Stephan Rohr]

From: "Rohr, Stephan" <stephan.rohr@intel.com>

'get_prev_frame_maybe_check_cycle' retrieves the previous frame and
computes the frame id.  At this point, the frame id is not cached yet,
which prevents the frame from being reinflated once the frane cache is
flushed.  Explicitly update the cache information before returning
'prev_frame' so the frame can be reinflated properly.

The 'up' command invokes the following sequence.

  1. Build the 'prev_frame' and eventually calls
     'get_prev_frame_maybe_check_cycle'.  The frame_id is only computed
     for the raw frame pointer, the wrapping 'frame_info_ptr' it not
     updated.
  2. Printing the frame calls 'do_print_frame_info'.
  3. If a pretty-printer runs an inferior call, the frame cache is
     flushed.
  4. This triggers the assertion when reinflating the frame_info_ptr.

Extend the 'gdb.python/pretty-print-call-by-hand.exp' testcase to cover
this use-case.
---
 gdb/frame.c                                    | 18 +++++++++++++++++-
 gdb/frame.h                                    |  4 ++++
 .../gdb.python/pretty-print-call-by-hand.exp   |  2 ++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 4137e1d5edd..1d4e231ea81 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2332,6 +2332,12 @@ get_prev_frame_maybe_check_cycle (const frame_info_ptr &this_frame)
       throw;
     }
 
+  /* No cycle and the frame is added to the frame stash, update the cache
+     information to ensure the frame can be reinflated after the cache is
+     flushed.  */
+  if (prev_frame)
+    prev_frame.update_cache ();
+
   return prev_frame;
 }
 
@@ -3431,7 +3437,17 @@ frame_info_ptr::frame_info_ptr (struct frame_info *ptr)
   if (m_ptr == nullptr)
     return;
 
-  m_cached_level = ptr->level;
+  update_cache ();
+}
+
+/* See frame-info-ptr.h.  */
+
+void
+frame_info_ptr::update_cache ()
+{
+  gdb_assert (m_ptr != nullptr);
+
+  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;
diff --git a/gdb/frame.h b/gdb/frame.h
index f6553fb7b6d..897518371b2 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -332,6 +332,10 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
     m_ptr = nullptr;
   }
 
+  /* Update the cached copies of frame_id and the frame level based on the
+     underlying pointer.   */
+  void update_cache ();
+
 private:
   /* We sometimes need to construct frame_info_ptr objects around the
      sentinel_frame, which has level -1.  Therefore, make the invalid frame
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.*"]
     }
 }
 
-- 
2.43.0

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

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

[Tom Tromey]

>>>>> Stephan Rohr <stephan.rohr@intel.com> writes:

> From: "Rohr, Stephan" <stephan.rohr@intel.com>
> 'get_prev_frame_maybe_check_cycle' retrieves the previous frame and
> computes the frame id.  At this point, the frame id is not cached yet,
> which prevents the frame from being reinflated once the frane cache is

s/frane/frame/

>   1. Build the 'prev_frame' and eventually calls
>      'get_prev_frame_maybe_check_cycle'.  The frame_id is only computed
>      for the raw frame pointer, the wrapping 'frame_info_ptr' it not
>      updated.

s/it not/is not/

> +  /* No cycle and the frame is added to the frame stash, update the cache
> +     information to ensure the frame can be reinflated after the cache is
> +     flushed.  */
> +  if (prev_frame)
> +    prev_frame.update_cache ();
> +

I wonder if some other approach could be taken here.
It seems unfortunate to have to add a new public method to this class
just to work around some sort of ordering issue.

That said I'm not sure I understand the problem well enough to suggest a
better solution.  Like should maybe get_prev_frame_raw not return a
frame_info_ptr?  And then the frame_info_ptr be created after the call
to frame_stash_add in get_prev_frame_maybe_check_cycle?

Tom

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

[Rohr, Stephan]

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.

Thanks
Stephan

> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Friday, 26 June 2026 17:07
> To: Rohr, Stephan <stephan.rohr@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb: set the cache information in
> 'get_prev_frame_maybe_check_cycle'
> 
> >>>>> Stephan Rohr <stephan.rohr@intel.com> writes:
> 
> > From: "Rohr, Stephan" <stephan.rohr@intel.com>
> > 'get_prev_frame_maybe_check_cycle' retrieves the previous frame and
> > computes the frame id.  At this point, the frame id is not cached yet,
> > which prevents the frame from being reinflated once the frane cache is
> 
> s/frane/frame/
> 
> >   1. Build the 'prev_frame' and eventually calls
> >      'get_prev_frame_maybe_check_cycle'.  The frame_id is only computed
> >      for the raw frame pointer, the wrapping 'frame_info_ptr' it not
> >      updated.
> 
> s/it not/is not/
> 
> > +  /* No cycle and the frame is added to the frame stash, update the cache
> > +     information to ensure the frame can be reinflated after the cache is
> > +     flushed.  */
> > +  if (prev_frame)
> > +    prev_frame.update_cache ();
> > +
> 
> I wonder if some other approach could be taken here.
> It seems unfortunate to have to add a new public method to this class
> just to work around some sort of ordering issue.
> 
> That said I'm not sure I understand the problem well enough to suggest a
> better solution.  Like should maybe get_prev_frame_raw not return a
> frame_info_ptr?  And then the frame_info_ptr be created after the call
> to frame_stash_add in get_prev_frame_maybe_check_cycle?
> 
> Tom
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

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

[Andrew Burgess]

"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 ();
+
   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;
+}
+
+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;
+
+  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.*"]
     }
 }
 

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

[Rohr, Stephan]

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.

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

> +}
> +
> +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?

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

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

[Andrew Burgess]

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

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

[Rohr, Stephan]

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

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

[Andrew Burgess]

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

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

[Rohr, Stephan]

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

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

[Andrew Burgess]

"Rohr, Stephan" <stephan.rohr@intel.com> 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 <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. 
>

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 <aburgess@redhat.com>
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 <stephan.rohr@intel.com>

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

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

[Rohr, Stephan]

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 <aburgess@redhat.com>
> Sent: Tuesday, 14 July 2026 17:14
> 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,
> >
> > 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 <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.
> >
> 
> 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 <aburgess@redhat.com>
> 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 <stephan.rohr@intel.com>
> 
> 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

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

[Andrew Burgess]

"Rohr, Stephan" <stephan.rohr@intel.com> writes:

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

Sorry about that, I guess I lost track of the original proposal
somewhere along the way.

I've gone ahead and pushed this patch for now.  I ran into this issue
again with another a second series that I'm working on, so I'd like to
see this fix landed, and it looks like we're both OK with this approach.

I agree there are some minor costs, but I don't see them as significant,
and anything else will also have some cost associated anyway, so I don't
think this is obviously that much worse than anything else.

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

Agreed.  I have this down as something to look at in the future.  I have
another series in progress that touches frame related stuff, so I need
to see how that will go first, but after that I might revisit this.

Thanks,
Andrew

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

[Tom Tromey]

Andrew> Comparing to null_frame_id always returns false, see
Andrew> frame_id::operator== for details.

I wonder if this could be changed.

This comparison behavior seems difficult to use, to me, whereas
comparing against "null" seems more normal... in ordinary code it's
really only NaNs that have this weird property and I tend to think
that's a bad model.

Tom

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

[Andrew Burgess]

"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'd like to dig into this a little more.  Why do you feel changing these
to take a raw pointer would be a bad thing?

Looking at the code again, I don't think it makes much sense to have
these take a frame_info_ptr.  A frame_info_ptr is a mechanism to cache a
frame-id and frame-level so that we can re-find a frame if the cache
gets cleared.

But having get_prev_frame_raw return a frame_info_ptr makes no sense (to
me), we know at this point that the frame doesn't have a frame-id, so
there is no way that the frame_info_ptr can ever do its job.

Similarly for compute_frame_id, we know the frame we are passing in
doesn't have a frame-id yet, so why pass it a frame_info_ptr?

I wonder if we change these functions to take a raw frame_info* then we
might (I haven't tried it yet) be able to have the frame_info_ptr
constructor assert that the frame_info* it is holding has a valid
frame_id, which we cannot right now, at least in part because of these
two functions.

Anyway, I just wondered if you'd seen some problem with changing these
functions that I was missing?

Thanks,
Andrew

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

[Rohr, Stephan]

Hello Andrew,

I think changing 'get_prev_frame_raw' to return a raw pointer
and changing 'compute_frame_id' to accept a raw pointer seems
reasonable.

I was hesitant to propose this change as calling
'frame_unwind_find_by_frame' in 'compute_frame_id' requires
a frame_info_ptr.  Same for ' fi->unwind->this_id (..)'.  It feels wrong
to create a temporary frame_info_ptr for these calls, though I observed
similar patterns, e.g., as done in 'create_new_frame'.

Changing 'get_prev_frame_maybe_check_cycle' to create a frame_info_ptr
object after its ID is computed certainly fixes the bug.

Thanks
Stephan

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Tuesday, 7 July 2026 12:34
> 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'd like to dig into this a little more.  Why do you feel changing these
> to take a raw pointer would be a bad thing?
> 
> Looking at the code again, I don't think it makes much sense to have
> these take a frame_info_ptr.  A frame_info_ptr is a mechanism to cache a
> frame-id and frame-level so that we can re-find a frame if the cache
> gets cleared.
> 
> But having get_prev_frame_raw return a frame_info_ptr makes no sense (to
> me), we know at this point that the frame doesn't have a frame-id, so
> there is no way that the frame_info_ptr can ever do its job.
> 
> Similarly for compute_frame_id, we know the frame we are passing in
> doesn't have a frame-id yet, so why pass it a frame_info_ptr?
> 
> I wonder if we change these functions to take a raw frame_info* then we
> might (I haven't tried it yet) be able to have the frame_info_ptr
> constructor assert that the frame_info* it is holding has a valid
> frame_id, which we cannot right now, at least in part because of these
> two functions.
> 
> Anyway, I just wondered if you'd seen some problem with changing these
> functions that I was missing?
> 
> Thanks,
> Andrew

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

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

[Andrew Burgess]

"Rohr, Stephan" <stephan.rohr@intel.com> writes:

> Hello Andrew,
>
> I think changing 'get_prev_frame_raw' to return a raw pointer
> and changing 'compute_frame_id' to accept a raw pointer seems
> reasonable.
>
> I was hesitant to propose this change as calling
> 'frame_unwind_find_by_frame' in 'compute_frame_id' requires
> a frame_info_ptr.  Same for ' fi->unwind->this_id (..)'.  It feels wrong
> to create a temporary frame_info_ptr for these calls, though I observed
> similar patterns, e.g., as done in 'create_new_frame'.

Yeah, after sending the email I started implementing this change and ran
into this exact case.  I agree that changing frame_unwind_find_by_frame
probably isn't going to be ideal right now, so I think my original
suggestion might be the way to go.

I think long term it might be right to downgrade some part of the frame
building API back to raw frame_info pointers, but I think this will need
some wider changes to prevent those pointers becoming invalidated,
e.g. frame_unwind_find_by_frame can invoke Python code, which can then
invalidate the frame cache (yuck), and though that probably is never
going to do anything useful, right now we run into problems reinflating
(due to missing frame-ids), and if we switch to raw frame_info pointer
we end up with use after free issues.

I'll go through your feedback to my earlier patch, and write up a commit
message and repost it shortly.

Thanks,
Andrew