This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH v3 4/4] Add a TRY_CATCH to get_prev_frame to better handle errors during unwind.


Here a new TRY_CATCH is added to the core of get_prev_frame, all uncaught
errors are turned into UNWIND_MISC_ERROR, and where possible the error
message associated with the error is stored as a frame specific stop reason
string.  The reason string is held of the frame OBSTACK so it lives as long
as the frame does.

There's a new function for getting the frame_stop_reason_string, this
replaces the now (thanks to patch #3) old frame_stop_reason_string
function, I know that reusing the name could confuse, but this function was
not widely used, so I hope that'll not be an issue.

Finally, I update the expected results from the tests in patch #1 now that
everything should pass.

I don't know if you'll all be happy with me catching _all_ errors in
get_prev_frame.  Given that the issues I'm seeing "in the wild" and which
I've put into the tests are for accessing invalid memory through a bad
stack pointer, I could reduce the catching to only catch MEMORY_ERRORs,
then UNWIND_MISC_ERROR would become UNWIND_MEMORY_ERROR.  I could even add
the memory error detection on top of the new MISC_ERROR case if preferred,
though I'd need to think of a new test...

Feedback welcome, or is this OK?

Thanks,
Andrew




gdb/ChangeLog:

	* frame.c (struct frame_info): Add stop_string field.
	(get_prev_frame_2): Renamed from get_prev_frame_1.
	(get_prev_frame_1): Old content moved into get_prev_frame_2.  Call
	get_prev_frame_2 inside TRY_CATCH, handle errors.
	(frame_stop_reason_string): New function definition.
	* frame.h (frame_stop_reason_string): New function declaration.
	* stack.c (frame_info): Switch to frame_stop_reason_string.
	(backtrace_command_1): Switch to frame_stop_reason_string.
	* unwind_stop_reason.def: Add UNWIND_MISC_ERROR.

gdb/testsuite/ChangeLog:

	* gdb.arch/amd64-invalid-stack-middle.exp: Update expected results.
	* gdb.arch/amd64-invalid-stack-top.exp: Likewise.
---
 gdb/frame.c                                        | 69 ++++++++++++++++++++--
 gdb/frame.h                                        |  6 ++
 gdb/stack.c                                        |  4 +-
 .../gdb.arch/amd64-invalid-stack-middle.exp        | 22 +------
 gdb/testsuite/gdb.arch/amd64-invalid-stack-top.exp | 22 +------
 gdb/unwind_stop_reasons.def                        |  5 +-
 6 files changed, 81 insertions(+), 47 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 5a6e0c7..65ba49b 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -146,6 +146,10 @@ struct frame_info
   /* The reason why we could not set PREV, or UNWIND_NO_REASON if we
      could.  Only valid when PREV_P is set.  */
   enum unwind_stop_reason stop_reason;
+
+  /* A frame specific string describing the STOP_REASON in more detail.
+     Only valid when PREV_P is set, but even then may still be NULL.  */
+  const char *stop_string;
 };
 
 /* A frame stash used to speed up frame lookups.  Create a hash table
@@ -1793,14 +1797,12 @@ get_prev_frame_if_no_cycle (struct frame_info *this_frame)
   return prev_frame;
 }
 
-/* Return a "struct frame_info" corresponding to the frame that called
-   THIS_FRAME.  Returns NULL if there is no such frame.
-
-   Unlike get_prev_frame, this function always tries to unwind the
-   frame.  */
+/* Helper function for get_prev_frame_1, this is called inside a
+   TRY_CATCH block.  Return the frame that called THIS_FRAME or NULL if
+   there is no such frame.  This may throw an exception.  */
 
 static struct frame_info *
-get_prev_frame_1 (struct frame_info *this_frame)
+get_prev_frame_2 (struct frame_info *this_frame)
 {
   struct gdbarch *gdbarch;
 
@@ -1950,6 +1952,41 @@ get_prev_frame_1 (struct frame_info *this_frame)
   return get_prev_frame_if_no_cycle (this_frame);
 }
 
+/* Return a "struct frame_info" corresponding to the frame that called
+   THIS_FRAME.  Returns NULL if there is no such frame.
+
+   Unlike get_prev_frame, this function always tries to unwind the
+   frame.  */
+
+static struct frame_info *
+get_prev_frame_1 (struct frame_info *this_frame)
+{
+  volatile struct gdb_exception ex;
+  struct frame_info *prev_frame = NULL;
+
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      prev_frame = get_prev_frame_2 (this_frame);
+    }
+  if (ex.reason < 0)
+    {
+      this_frame->stop_reason = UNWIND_MISC_ERROR;
+      if (ex.message != NULL)
+	{
+	  char *stop_string;
+
+	  /* The error needs to live as long as the frame does.  */
+	  stop_string =
+	    FRAME_OBSTACK_CALLOC (strlen (ex.message) + 1, char);
+	  strcpy (stop_string, ex.message);
+	  this_frame->stop_string = stop_string;
+	}
+      prev_frame = NULL;
+    }
+
+  return prev_frame;
+}
+
 /* Construct a new "struct frame_info" and link it previous to
    this_frame.  */
 
@@ -2571,6 +2608,26 @@ deprecated_frame_stop_reason_string (enum unwind_stop_reason reason)
     }
 }
 
+/* Return a possibly frame specific string explaining why the unwind
+   stopped at frame FI.  Must only be called if there is no previous
+   frame.  */
+
+const char *
+frame_stop_reason_string (struct frame_info *fi)
+{
+  gdb_assert (fi->prev_p);
+  gdb_assert (fi->prev == NULL);
+
+  /* Return the specific string if we have one.  */
+  if (fi->stop_string)
+    return fi->stop_string;
+
+  /* Return the generic string if we have nothing better.  At some point
+     if DEPRECATED_FRAME_STOP_REASON_STRING is not used anywhere else then
+     its content could be moved into here.  */
+  return deprecated_frame_stop_reason_string (fi->stop_reason);
+}
+
 /* Return the enum symbol name of REASON as a string, to use in debug
    output.  */
 
diff --git a/gdb/frame.h b/gdb/frame.h
index 7db5382..1a5614d 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -500,6 +500,12 @@ enum unwind_stop_reason get_frame_unwind_stop_reason (struct frame_info *);
 
 const char *deprecated_frame_stop_reason_string (enum unwind_stop_reason);
 
+/* Return a possibly frame specific string explaining why the unwind
+   stopped here.  Should only be called for frames that don't have a
+   previous frame.  If there's no specific reason stored for a frame then
+   a generic reason string will be returned.  */
+const char *frame_stop_reason_string (struct frame_info *);
+
 /* Unwind the stack frame so that the value of REGNUM, in the previous
    (up, older) frame is returned.  If VALUEP is NULL, don't
    fetch/compute the value.  Instead just return the location of the
diff --git a/gdb/stack.c b/gdb/stack.c
index 4fa9dd6..3f8ecfb 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1528,7 +1528,7 @@ frame_info (char *addr_exp, int from_tty)
       reason = get_frame_unwind_stop_reason (fi);
       if (reason != UNWIND_NO_REASON)
 	printf_filtered (_(" Outermost frame: %s\n"),
-			 deprecated_frame_stop_reason_string (reason));
+			 frame_stop_reason_string (fi));
     }
   else if (get_frame_type (fi) == TAILCALL_FRAME)
     puts_filtered (" tail call frame");
@@ -1847,7 +1847,7 @@ backtrace_command_1 (char *count_exp, int show_locals, int no_filters,
 	  reason = get_frame_unwind_stop_reason (trailing);
 	  if (reason >= UNWIND_FIRST_ERROR)
 	    printf_filtered (_("Backtrace stopped: %s\n"),
-			     deprecated_frame_stop_reason_string (reason));
+			     frame_stop_reason_string (trailing));
 	}
     }
 }
diff --git a/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp b/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp
index ef70a4f..b53ad41 100644
--- a/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp
+++ b/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp
@@ -43,27 +43,11 @@ if ![runto breakpt] {
     return -1
 }
 
-gdb_test "bt" "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nCannot access memory at address 0x\[0-9a-f\]+" \
+gdb_test "bt" "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nBacktrace stopped: Cannot access memory at address 0x\[0-9a-f\]+" \
 	 "first backtrace, with error message"
 
-send_gdb "bt\n"
-gdb_expect {
-    -re "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nCannot access memory at address 0x\[0-9a-f\]+\r\n$gdb_prompt $" {
-	# Currently gdb will not display the error message associated with
-	# the truncated backtrace after the first backtrace has been
-	# completed.  Ideally, we would do this.  If this case is ever hit
-	# then we have started to display the backtrace in all cases and
-	# the xpass should becomd a pass, and the previous pass case below
-	# should be removed, or changed to a fail.
-	xpass "second backtrace, with error message"
-    }
-    -re "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\n$gdb_prompt $" {
-	pass "second backtrace, without error message"
-    }
-    timeout {
-	fail "second backtrace (timeout)"
-    }
-}
+gdb_test "bt" "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nBacktrace stopped: Cannot access memory at address 0x\[0-9a-f\]+" \
+	 "second backtrace, with error message"
 
 clean_restart ${binfile}
 
diff --git a/gdb/testsuite/gdb.arch/amd64-invalid-stack-top.exp b/gdb/testsuite/gdb.arch/amd64-invalid-stack-top.exp
index 6598cd4..52869a2 100644
--- a/gdb/testsuite/gdb.arch/amd64-invalid-stack-top.exp
+++ b/gdb/testsuite/gdb.arch/amd64-invalid-stack-top.exp
@@ -46,27 +46,11 @@ if ![runto breakpt] {
 # Use 'bt no-filters' here as the python filters will raise their own
 # error during initialisation, the no-filters case is simpler.
 
-gdb_test "bt no-filters" "^bt no-filters\r\n#0 +$hex in func2 \\(\\)\r\nCannot access memory at address 0x\[0-9a-f\]+" \
+gdb_test "bt no-filters" "^bt no-filters\r\n#0 +$hex in func2 \\(\\)\r\nBacktrace stopped: Cannot access memory at address 0x\[0-9a-f\]+" \
 	 "first backtrace, with error message"
 
-send_gdb "bt no-filters\n"
-gdb_expect {
-    -re "^bt no-filters\r\n#0 +$hex in func2 \\(\\)\r\nCannot access memory at address 0x\[0-9a-f\]+\r\n$gdb_prompt $" {
-	# Currently gdb will not display the error message associated with
-	# the truncated backtrace after the first backtrace has been
-	# completed.  Ideally, we would do this.  If this case is ever hit
-	# then we have started to display the backtrace in all cases and
-	# the xpass should becomd a pass, and the previous pass case below
-	# should be removed, or changed to a fail.
-	xpass "second backtrace, with error message"
-    }
-    -re "^bt no-filters\r\n#0 +$hex in func2 \\(\\)\r\n$gdb_prompt $" {
-	pass "second backtrace, without error message"
-    }
-    timeout {
-	fail "second backtrace (timeout)"
-    }
-}
+gdb_test "bt no-filters" "^bt no-filters\r\n#0 +$hex in func2 \\(\\)\r\nBacktrace stopped: Cannot access memory at address 0x\[0-9a-f\]+" \
+	 "second backtrace, with error message"
 
 clean_restart ${binfile}
 
diff --git a/gdb/unwind_stop_reasons.def b/gdb/unwind_stop_reasons.def
index d7da7ea..84cfb75 100644
--- a/gdb/unwind_stop_reasons.def
+++ b/gdb/unwind_stop_reasons.def
@@ -60,6 +60,9 @@ SET (UNWIND_SAME_ID, "previous frame identical to this frame (corrupt stack?)")
    one to unwind further.  */
 SET (UNWIND_NO_SAVED_PC, "frame did not save the PC")
 
+/* The frame unwinder threw some other error which we caught.  */
+SET (UNWIND_MISC_ERROR, "miscellaneous error while unwinding")
+
 #endif /* SET */
 
 
@@ -72,5 +75,5 @@ FIRST_ENTRY (UNWIND_NO_REASON)
 #endif
 
 #ifdef LAST_ENTRY
-LAST_ENTRY (UNWIND_NO_SAVED_PC)
+LAST_ENTRY (UNWIND_MISC_ERROR)
 #endif
-- 
1.8.1.3


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]