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

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

Hi Andrew,

thanks for sharing 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