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 cefdde5ed1ef9b5428e1e6dad3688989a653c35b..b91e18fad9946ddfe4a129d762b5e8c3ea342845 100644 (file)
--- 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 52162fc995220cc831f977cd158b3e7e9a120a34..a2a29c4d0f87c715e457b29c4b1af02b1178453f 100644 (file)
--- 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.*"]
     }
 }