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

Message ID 1398855344-25278-5-git-send-email-aburgess@broadcom.com
State Superseded
Headers

Commit Message

Andrew Burgess April 30, 2014, 10:55 a.m. UTC
  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(-)
  

Comments

Pedro Alves May 28, 2014, 6:31 p.m. UTC | #1
On 04/30/2014 11:55 AM, Andrew Burgess wrote:
> 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.

Sounds like we'll want to expose this to Python too.  If you're not
planning on doing that, could you file a PR once this goes in?

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

Yeah, that makes me somewhat nervous.  E.g., this catches and
swallows TARGET_CLOSE_ERROR.

> 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 think I'd prefer that.  This swallows TARGET_CLOSE_ERROR, for example.

Could you please include both MI and CLI before/after examples
in the description/commit log, please?  You had something
close to that in the cover letter, but that doesn't make it to
the repository.


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

I didn't understand this.

> 
> Feedback welcome, or is this OK?

> 	* unwind_stop_reason.def: Add UNWIND_MISC_ERROR.

Values added here are exposed to Python/scheme automatically.
But, we need to document them manually in the manual.  E.g.
for python, in gdb/python.texi in the table beneath the
description of Frame.unwind_stop_reason ().
  

Patch

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