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

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

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