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

[Andrew Burgess <aburgess@redhat.com>]

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

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