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

Andrew Burgess
2014-04-30T10:55:55Z

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