gdb: avoid premature dummy frame garbage collection

Consider the following chain of events:

  * GDB is performing an inferior call, and

  * the inferior calls longjmp, and

  * GDB detects that the longjmp has completed, stops, and enters
    check_longjmp_breakpoint_for_call_dummy (in breakpoint.c), and

  * GDB tries to unwind the stack in order to check that the dummy
    frame (setup for the inferior call) is still on the stack, but

  * The unwind fails, possibly due to missing debug information, so

  * GDB incorrectly concludes that the inferior has longjmp'd past the
    dummy frame, and so deletes the dummy frame, including the dummy
    frame breakpoint, but then

  * The inferior continues, and eventually returns to the dummy frame,
    which is usually (always?) on the stack, the inferior starts
    trying to execute the random contents of the stack, this results
    in undefined behaviour.

This situation is already warned about in the comment on the function
check_longjmp_breakpoint_for_call_dummy where we say:

   You should call this function only at places where it is safe to currently
   unwind the whole stack.  Failed stack unwind would discard live dummy
   frames.

The warning here is fine, the problem is that, even though we call the
function from a location within GDB where we hope to be able to
unwind, sometime the state of the inferior means that the unwind will
not succeed.

This commit tries to improve the situation by adding the following
additional check; when GDB fails to find the dummy frame on the stack,
instead of just assuming that the dummy frame can be garbage
collected, first find the stop_reason for the last frame on the stack.
If this stop_reason indicates that the stack unwinding may have failed
then we assume that the dummy frame is still in use.  However, if the
last frame's stop_reason indicates that the stack unwind completed
successfully then we can be confident that the dummy frame is no
longer in use, and we garbage collect it.

Tested on x86-64 GNU/Linux.

gdb/ChangeLog:

	* breakpoint.c (check_longjmp_breakpoint_for_call_dummy): Add
	check for why the backtrace stopped.

gdb/testsuite/ChangeLog:

	* gdb.base/premature-dummy-frame-removal.c: New file.
	* gdb.base/premature-dummy-frame-removal.exp: New file.
	* gdb.base/premature-dummy-frame-removal.py: New file.

Change-Id: I8f330cfe0f3f33beb3a52a36994094c4abada07e

6 files changed, 238 insertions, 4 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7581c09..b6930df 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2021-06-01  Andrew Burgess  <andrew.burgess@embecosm.com>
+	    Richard Bunt  <richard.bunt@arm.com>
+
+	* breakpoint.c (check_longjmp_breakpoint_for_call_dummy): Add
+	check for why the backtrace stopped.
+
 2021-05-31  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	* dwarf2/read.h (struct structured_type) <signatured_type>: New.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index aa6b348..fb011fc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7357,9 +7357,10 @@ set_longjmp_breakpoint_for_call_dummy (void)
    TP.  Remove those which can no longer be found in the current frame
    stack.
 
-   You should call this function only at places where it is safe to currently
-   unwind the whole stack.  Failed stack unwind would discard live dummy
-   frames.  */
+   If the unwind fails then there is not sufficient information to discard
+   dummy frames.  In this case, elide the clean up and the dummy frames will
+   be cleaned up next time this function is called from a location where
+   unwinding is possible.  */
 
 void
 check_longjmp_breakpoint_for_call_dummy (struct thread_info *tp)
@@ -7371,12 +7372,55 @@ check_longjmp_breakpoint_for_call_dummy (struct thread_info *tp)
       {
 	struct breakpoint *dummy_b = b->related_breakpoint;
 
+	/* Find the bp_call_dummy breakpoint in the list of breakpoints
+	   chained off b->related_breakpoint.  */
 	while (dummy_b != b && dummy_b->type != bp_call_dummy)
 	  dummy_b = dummy_b->related_breakpoint;
+
+	/* If there was no bp_call_dummy breakpoint then there's nothing
+	   more to do.  Or, if the dummy frame associated with the
+	   bp_call_dummy is still on the stack then we need to leave this
+	   bp_call_dummy in place.  */
 	if (dummy_b->type != bp_call_dummy
 	    || frame_find_by_id (dummy_b->frame_id) != NULL)
 	  continue;
-	
+
+	/* We didn't find the dummy frame on the stack, this could be
+	   because we have longjmp'd to a stack frame that is previous to
+	   the dummy frame, or it could be because the stack unwind is
+	   broken at some point between the longjmp frame and the dummy
+	   frame.
+
+	   Next we figure out why the stack unwind stopped.  If it looks
+	   like the unwind is complete then we assume the dummy frame has
+	   been jumped over, however, if the unwind stopped for an
+	   unexpected reason then we assume the stack unwind is currently
+	   broken, and that we will (eventually) return to the dummy
+	   frame.
+
+	   It might be tempting to consider using frame_id_inner here, but
+	   that is not safe.   There is no guarantee that the stack frames
+	   we are looking at here are even on the same stack as the
+	   original dummy frame, hence frame_id_inner can't be used.  See
+	   the comments on frame_id_inner for more details.  */
+	bool unwind_finished_unexpectedly = false;
+	for (struct frame_info *fi = get_current_frame (); fi != nullptr; )
+	  {
+	    struct frame_info *prev = get_prev_frame (fi);
+	    if (prev == nullptr)
+	      {
+		/* FI is the last stack frame.  Why did this frame not
+		   unwind further?  */
+		auto stop_reason = get_frame_unwind_stop_reason (fi);
+		if (stop_reason != UNWIND_NO_REASON
+		    && stop_reason != UNWIND_OUTERMOST)
+		  unwind_finished_unexpectedly = true;
+	      }
+	    fi = prev;
+	  }
+	if (unwind_finished_unexpectedly)
+	  continue;
+
 	dummy_frame_discard (dummy_b->frame_id, tp);
 
 	while (b->related_breakpoint != b)
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index e2a95d48..6c70793 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2021-06-01  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.base/premature-dummy-frame-removal.c: New file.
+	* gdb.base/premature-dummy-frame-removal.exp: New file.
+	* gdb.base/premature-dummy-frame-removal.py: New file.
+
 2021-05-27  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	* gdb.base/reverse-init-functions.exp: New.
diff --git a/gdb/testsuite/gdb.base/premature-dummy-frame-removal.c b/gdb/testsuite/gdb.base/premature-dummy-frame-removal.c
new file mode 100644
index 0000000..32deefc
--- /dev/null
+++ b/gdb/testsuite/gdb.base/premature-dummy-frame-removal.c
@@ -0,0 +1,65 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <setjmp.h>
+
+jmp_buf env;
+
+void
+worker (void)
+{
+  longjmp (env, 1);
+}
+
+void
+test_inner (void)
+{
+  if (setjmp (env) == 0)
+    {
+      /* Direct call.  */
+      worker ();
+
+      /* Will never get here.  */
+      abort ();
+    }
+  else
+    {
+      /* Called from longjmp.  */
+    }
+}
+
+void
+break_bt_here (void)
+{
+  test_inner ();
+}
+
+int
+some_func (void)
+{
+  break_bt_here ();
+  return 0;
+}
+
+int
+main (void)
+{
+  some_func ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/premature-dummy-frame-removal.exp b/gdb/testsuite/gdb.base/premature-dummy-frame-removal.exp
new file mode 100644
index 0000000..bf2a2a7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/premature-dummy-frame-removal.exp
@@ -0,0 +1,53 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Make an inferior call to a function which uses longjmp.  However,
+# the backtrace for the function that is called is broken at the point
+# where the longjmp is handled.  This test is checking to see if the
+# inferior call still completes successfully.
+#
+# This test forces a broken backtrace using Python, but in real life a
+# broken backtrace can easily occur when calling through code for
+# which there is no debug information if the prologue unwinder fails,
+# which can often happen if the code has been optimized.
+#
+# The problem was that, due to the broken backtrace, GDB failed to
+# find the inferior call's dummy frame.  GDB then concluded that the
+# inferior had longjmp'd backward past the dummy frame and so garbage
+# collected the dummy frame, this causes the breakpoint within the
+# dummy frame to be deleted.
+#
+# When the inferior continued, and eventually returned to the dummy
+# frame, it would try to execute instruction from the dummy frame
+# (which for most, or even all, targets, is on the stack), and then
+# experience undefined behaviuor, often a SIGSEGV.
+
+standard_testfile .c
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+if ![runto_main] then {
+    return 0
+}
+
+# Skip this test if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+gdb_test_no_output "source ${pyfile}" "load python file"
+
+gdb_test "p some_func ()" " = 0"
diff --git a/gdb/testsuite/gdb.base/premature-dummy-frame-removal.py b/gdb/testsuite/gdb.base/premature-dummy-frame-removal.py
new file mode 100644
index 0000000..3193665
--- /dev/null
+++ b/gdb/testsuite/gdb.base/premature-dummy-frame-removal.py
@@ -0,0 +1,60 @@
+# Copyright (C) 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This dummy unwinder will break GDB's backtrce at the function called
+# 'break_bt_here'.
+
+import gdb
+from gdb.unwinder import Unwinder
+
+
+class FrameId(object):
+    def __init__(self, sp, pc):
+        self._sp = sp
+        self._pc = pc
+
+    @property
+    def sp(self):
+        return self._sp
+
+    @property
+    def pc(self):
+        return self._pc
+
+
+class TestUnwinder(Unwinder):
+    def __init__(self):
+        Unwinder.__init__(self, "break unwinding")
+
+    def __call__(self, pending_frame):
+        pc_desc = pending_frame.architecture().registers().find("pc")
+        pc = pending_frame.read_register(pc_desc)
+
+        sp_desc = pending_frame.architecture().registers().find("sp")
+        sp = pending_frame.read_register(sp_desc)
+
+        block = gdb.block_for_pc(int(pc))
+        if block == None:
+            return None
+        func = block.function
+        if func == None:
+            return None
+        if str(func) != "break_bt_here":
+            return None
+        fid = FrameId(pc, sp)
+        return pending_frame.create_unwind_info(fid)
+
+
+gdb.unwinder.register_unwinder(None, TestUnwinder(), True)