Skip to content

Commit

Permalink
Fix frame cycle detection
Browse files Browse the repository at this point in the history 
The recent commit to make scoped_restore_current_thread's cdtors
exception free regressed gdb.base/eh_return.exp:

  Breakpoint 1, 0x00000000004012bb in eh2 (gdb/frame.c:641: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  Quit this debugging session? (y or n) FAIL: gdb.base/eh_return.exp: hit breakpoint (GDB internal error)

That testcase uses __builtin_eh_return and, before the regression, the
backtrace at eh2 looked like this:

 (gdb) bt
 #0  0x00000000004006eb in eh2 (p=0x4006ec <continuation>) at src/gdb/testsuite/gdb.base/eh_return.c:54
 Backtrace stopped: previous frame identical to this frame (corrupt stack?)

That "previous frame identical to this frame" is caught by the cycle
detection based on frame id.

The assertion failing is this one:

 638           /* Since this is the first frame in the chain, this should
 639              always succeed.  */
 640           bool stashed = frame_stash_add (fi);
 641           gdb_assert (stashed);

originally added by

  commit f245535
  Author:     Pedro Alves <[email protected]>
  AuthorDate: Mon Sep 5 18:41:38 2016 +0100

      Fix PR19927: Avoid unwinder recursion if sniffer uses calls parse_and_eval

The assertion is failing because frame #1's frame id was stashed
before the id of frame #0 is stashed.  The frame id of frame #1 was
stashed here:

 (top-gdb) bt
 #0  frame_stash_add (frame=0x1e24c90) at src/gdb/frame.c:276
 #1  0x0000000000669c1b in get_prev_frame_if_no_cycle (this_frame=0x19f8370) at src/gdb/frame.c:2120
 #2  0x000000000066a339 in get_prev_frame_always_1 (this_frame=0x19f8370) at src/gdb/frame.c:2303
 #3  0x000000000066a360 in get_prev_frame_always (this_frame=0x19f8370) at src/gdb/frame.c:2319
 #4  0x000000000066b56c in get_frame_unwind_stop_reason (frame=0x19f8370) at src/gdb/frame.c:3028
 #5  0x000000000059f929 in dwarf2_frame_cfa (this_frame=0x19f8370) at src/gdb/dwarf2/frame.c:1462
 #6  0x00000000005ce434 in dwarf_evaluate_loc_desc::get_frame_cfa (this=0x7fffffffc070) at src/gdb/dwarf2/loc.c:666
 #7  0x00000000005989a9 in dwarf_expr_context::execute_stack_op (this=0x7fffffffc070, op_ptr=0x1b2a053 "\364\003", op_end=0x1b2a053 "\364\003") at src/gdb/dwarf2/expr.c:1161
 #8  0x0000000000596af6 in dwarf_expr_context::eval (this=0x7fffffffc070, addr=0x1b2a052 "\234\364\003", len=1) at src/gdb/dwarf2/expr.c:303
 #9  0x0000000000597b4e in dwarf_expr_context::execute_stack_op (this=0x7fffffffc070, op_ptr=0x1b2a063 "", op_end=0x1b2a063 "") at src/gdb/dwarf2/expr.c:865
 #10 0x0000000000596af6 in dwarf_expr_context::eval (this=0x7fffffffc070, addr=0x1b2a061 "\221X", len=2) at src/gdb/dwarf2/expr.c:303
 #11 0x00000000005c8b5a in dwarf2_evaluate_loc_desc_full (type=0x1b564d0, frame=0x19f8370, data=0x1b2a061 "\221X", size=2, per_cu=0x1b28760, per_objfile=0x1a84930, subobj_type=0x1b564d0, subobj_byte_offset=0) at src/gdb/dwarf2/loc.c:2260
 #12 0x00000000005c9243 in dwarf2_evaluate_loc_desc (type=0x1b564d0, frame=0x19f8370, data=0x1b2a061 "\221X", size=2, per_cu=0x1b28760, per_objfile=0x1a84930) at src/gdb/dwarf2/loc.c:2444
 #13 0x00000000005cb769 in locexpr_read_variable (symbol=0x1b59840, frame=0x19f8370) at src/gdb/dwarf2/loc.c:3687
 #14 0x0000000000663137 in language_defn::read_var_value (this=0x122ea60 <c_language_defn>, var=0x1b59840, var_block=0x0, frame=0x19f8370) at src/gdb/findvar.c:618
 #15 0x0000000000663c3b in read_var_value (var=0x1b59840, var_block=0x0, frame=0x19f8370) at src/gdb/findvar.c:822
 #16 0x00000000008c7d9f in read_frame_arg (fp_opts=..., sym=0x1b59840, frame=0x19f8370, argp=0x7fffffffc470, entryargp=0x7fffffffc490) at src/gdb/stack.c:542
 #17 0x00000000008c89cd in print_frame_args (fp_opts=..., func=0x1b597c0, frame=0x19f8370, num=-1, stream=0x1aba860) at src/gdb/stack.c:890
 #18 0x00000000008c9bf8 in print_frame (fp_opts=..., frame=0x19f8370, print_level=0, print_what=SRC_AND_LOC, print_args=1, sal=...) at src/gdb/stack.c:1394
 #19 0x00000000008c92b9 in print_frame_info (fp_opts=..., frame=0x19f8370, print_level=0, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1) at src/gdb/stack.c:1119
 #20 0x00000000008c75f0 in print_stack_frame (frame=0x19f8370, print_level=0, print_what=SRC_AND_LOC, set_current_sal=1) at src/gdb/stack.c:366
 #21 0x000000000070250b in print_stop_location (ws=0x7fffffffc9e0) at src/gdb/infrun.c:8110
 #22 0x0000000000702569 in print_stop_event (uiout=0x1a8b9e0, displays=true) at src/gdb/infrun.c:8126
 #23 0x000000000096d04b in tui_on_normal_stop (bs=0x1bcd1c0, print_frame=1) at src/gdb/tui/tui-interp.c:98
 ...

Before the commit to make scoped_restore_current_thread's cdtors
exception free, scoped_restore_current_thread's dtor would call
get_frame_id on the selected frame, and we use
scoped_restore_current_thread pervasively.  That had the side effect
of stashing the frame id of frame #0 before reaching the path shown in
the backtrace.  I.e., the frame id of frame #0 happened to be stashed
before the frame id of frame #1.  But that was by chance, not by
design.

This commit:

  commit 256ae5d
  Author:     Kevin Buettner <[email protected]>
  AuthorDate: Mon Oct 31 12:47:42 2016 -0700

      Stash frame id of current frame before stashing frame id for previous frame

Fixed a similar problem, by making sure get_prev_frame computes the
frame id of the current frame before unwinding the previous frame, so
that the cycle detection works properly.  That fix misses the scenario
we're now running against, because if you notice, the backtrace above
shows that frame #4 calls get_prev_frame_always, not get_prev_frame.
I.e., nothing is calling get_frame_id on the current frame.

The fix here is to move Kevin's fix down from get_prev_frame to
get_prev_frame_always.  Or actually, a bit further down to
get_prev_frame_always_1 -- note that inline_frame_this_id calls
get_prev_frame_always, so we need to be careful to avoid recursion in
that scenario.

gdb/ChangeLog:

	* frame.c (get_prev_frame): Move get_frame_id call from here ...
	(get_prev_frame_always_1): ... to here.
	* inline-frame.c (inline_frame_this_id): Mention
	get_prev_frame_always_1 in comment.

Change-Id: Id960c98ab2d072c48a436c3eb160cc4b2a5cfd1d
  • Loading branch information
@palves
palves committed Nov 16, 2020
1 parent 10c9f4e commit 2b3cb40
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 11 deletions.
7 changes: 7 additions & 0 deletions gdb/ChangeLog
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
2020-11-16 Pedro Alves <[email protected]>

* frame.c (get_prev_frame): Move get_frame_id call from here ...
(get_prev_frame_always_1): ... to here.
* inline-frame.c (inline_frame_this_id): Mention
get_prev_frame_always_1 in comment.

2020-11-15 Joel Brobecker <[email protected]>

* valarith.c (fixed_point_binop): Add BINOP_EQUAL and BINOP_LESS
Expand Down
27 changes: 17 additions & 10 deletions gdb/frame.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2200,6 +2200,23 @@ get_prev_frame_always_1 (struct frame_info *this_frame)
if (get_frame_type (this_frame) == INLINE_FRAME)
return get_prev_frame_if_no_cycle (this_frame);

/* If this_frame is the current frame, then compute and stash its
frame id prior to fetching and computing the frame id of the
previous frame. Otherwise, the cycle detection code in
get_prev_frame_if_no_cycle() will not work correctly. When
get_frame_id() is called later on, an assertion error will be
triggered in the event of a cycle between the current frame and
its previous frame.
Note we do this after the INLINE_FRAME check above. That is
because the inline frame's frame id computation needs to fetch
the frame id of its previous real stack frame. I.e., we need to
avoid recursion in that case. This is OK since we're sure the
inline frame won't create a cycle with the real stack frame. See
inline_frame_this_id. */
if (this_frame->level == 0)
get_frame_id (this_frame);

/* Check that this frame is unwindable. If it isn't, don't try to
unwind to the prev frame. */
this_frame->stop_reason
Expand Down Expand Up @@ -2492,16 +2509,6 @@ get_prev_frame (struct frame_info *this_frame)
something should be calling get_selected_frame() or
get_current_frame(). */
gdb_assert (this_frame != NULL);

/* If this_frame is the current frame, then compute and stash
its frame id prior to fetching and computing the frame id of the
previous frame. Otherwise, the cycle detection code in
get_prev_frame_if_no_cycle() will not work correctly. When
get_frame_id() is called later on, an assertion error will
be triggered in the event of a cycle between the current
frame and its previous frame. */
if (this_frame->level == 0)
get_frame_id (this_frame);

frame_pc_p = get_frame_pc_if_available (this_frame, &frame_pc);

Expand Down
3 changes: 2 additions & 1 deletion gdb/inline-frame.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ inline_frame_this_id (struct frame_info *this_frame,
real frame's this_id method. So we must call
get_prev_frame_always. Because we are inlined into some
function, there must be previous frames, so this is safe - as
long as we're careful not to create any cycles. */
long as we're careful not to create any cycles. See related
comments in get_prev_frame_always_1. */
*this_id = get_frame_id (get_prev_frame_always (this_frame));

/* We need a valid frame ID, so we need to be based on a valid
Expand Down

0 comments on commit 2b3cb40

Please sign in to comment.