Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully

Yao Qi
2016-05-04T16:24:30Z

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

After trying two different approaches of ultimate-fallback unwinder, and
think again about 'wrapping methods of struct frame_unwind', I think
'wrapping struct frame_unwind methods' still works, maybe.

> There are a few constraints that we need to keep in mind:
>
> - Frames that only have the PC available should have distinct frame ids,
> and it should be distinct from outer_frame_id. (See frame_id_build_unavailable_stack calls).
>

This can be done by wrapping fi->unwind->this_id with TRY/CATCH, call
get_frame_pc_if_available, if PC is available, call
frame_id_build_unavailable_stack otherwise, fall back to
outer_frame_id. I did that in my patch below,

> This makes e.g., the frame_id_eq check in tfind_1 work as intended, see:
> https://sourceware.org/ml/gdb-patches/2013-12/msg00535.html
>
> - When an unwind sniffer throws, it'll destroy its
> struct frame_unwind_cache. So if we don't catch the error, the
> frame's this_id method can't return something more detailed than
> outer_frame_id.

unwinder->sniffer is wrapped by TRY/CATCH nowadays, so we don't have to
change anything.

>
> I don't see this done by wrapping methods of 'struct frame_unwind'.

>
> I think it'd work to have an ultimate-fallback unwinder that
> frame_unwind_find_by_frame returns instead of the internal error at
> the end. This would return UNWIND_UNAVAILABLE or UNWIND_MEMORY_ERROR
> in the unwinder->stop_reason method, depending on the error the last registered
> unwinder thrown. (That last unwinder will always be the arch's
> heuristic unwinder.)

In my patch, this_frame->unwind->stop_reason is wrapped by TRY/CATCH,
and the stop_reason is set to UNWIND_UNAVAILABLE if error is
NOT_AVAILABLE_ERROR. I don't set stop_reason to UNWIND_MEMORY_ERROR as
you suggested, because I think it can be a follow-up improvement. Let
us focus on unavailable things first.

> And it would return frame_id_build_unavailable_stack(PC) in the unwinder->this_id
> method if the last error was UNWIND_UNAVAILABLE, outer_frame_id otherwise
> (or we add a new frame_id_build_stackless function, to go along with
> frame_id_build_unavailable_stack).

I think my patch can do this. The patch below is an RFC. Run
gdb.trace/*.exp tests on both x86_64-linux and aarch64-linux. What do
you think? This patch
https://sourceware.org/ml/gdb-patches/2016-04/msg00429.html is applied
locally to make sure x86 prologue unwinders are selected by gdb.

--
Yao (齐尧)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6f2e38e..52d89b7f 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -559,8 +559,7 @@ aarch64_make_prologue_cache (struct frame_info *this_frame, void **this_cache)
}
CATCH (ex, RETURN_MASK_ERROR)
{
- if (ex.error != NOT_AVAILABLE_ERROR)
- throw_exception (ex);
+ throw_exception (ex);
}
END_CATCH

@@ -687,8 +686,7 @@ aarch64_make_stub_cache (struct frame_info *this_frame, void **this_cache)
}
CATCH (ex, RETURN_MASK_ERROR)
{
- if (ex.error != NOT_AVAILABLE_ERROR)
- throw_exception (ex);
+ throw_exception (ex);
}
END_CATCH

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 0065523..192d27b 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2510,8 +2510,7 @@ amd64_frame_cache (struct frame_info *this_frame, void **this_cache)
}
CATCH (ex, RETURN_MASK_ERROR)
{
- if (ex.error != NOT_AVAILABLE_ERROR)
- throw_exception (ex);
+ throw_exception (ex);
}
END_CATCH

@@ -2638,8 +2637,7 @@ amd64_sigtramp_frame_cache (struct frame_info *this_frame, void **this_cache)
}
CATCH (ex, RETURN_MASK_ERROR)
{
- if (ex.error != NOT_AVAILABLE_ERROR)
- throw_exception (ex);
+ throw_exception (ex);
}
END_CATCH

@@ -2819,8 +2817,7 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
}
CATCH (ex, RETURN_MASK_ERROR)
{
- if (ex.error != NOT_AVAILABLE_ERROR)
- throw_exception (ex);
+ throw_exception (ex);
}
END_CATCH

diff --git a/gdb/frame.c b/gdb/frame.c
index d621dd7..60deca3 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -478,7 +478,26 @@ compute_frame_id (struct frame_info *fi)
/* Find THIS frame's ID. */
/* Default to outermost if no ID is found. */
fi->this_id.value = outer_frame_id;
- fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+
+ TRY
+ {
+ fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ if (ex.error == NOT_AVAILABLE_ERROR)
+ {
+ CORE_ADDR pc;
+
+ /* Fall back to outer_frame_id if PC isn't available. */
+ if (get_frame_pc_if_available (fi, &pc))
+ fi->this_id.value = frame_id_build_unavailable_stack (pc);
+ }
+ else
+ throw_exception (ex);
+ }
+ END_CATCH
+
gdb_assert (frame_id_p (fi->this_id.value));
fi->this_id.p = 1;
if (frame_debug)
@@ -1882,9 +1901,20 @@ get_prev_frame_always_1 (struct frame_info *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
- = this_frame->unwind->stop_reason (this_frame,
- &this_frame->prologue_cache);
+ TRY
+ {
+ this_frame->stop_reason
+ = this_frame->unwind->stop_reason (this_frame,
+ &this_frame->prologue_cache);
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ if (ex.error == NOT_AVAILABLE_ERROR)
+ this_frame->stop_reason = UNWIND_UNAVAILABLE;
+ else
+ throw_exception (ex);
+ }
+ END_CATCH

if (this_frame->stop_reason != UNWIND_NO_REASON)
{
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 83a4881..f52eb0f 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2074,8 +2074,7 @@ i386_frame_cache (struct frame_info *this_frame, void **this_cache)
}
CATCH (ex, RETURN_MASK_ERROR)
{
- if (ex.error != NOT_AVAILABLE_ERROR)
- throw_exception (ex);
+ throw_exception (ex);
}
END_CATCH

@@ -2254,8 +2253,7 @@ i386_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
}
CATCH (ex, RETURN_MASK_ERROR)
{
- if (ex.error != NOT_AVAILABLE_ERROR)
- throw_exception (ex);
+ throw_exception (ex);
}
END_CATCH

@@ -2450,8 +2448,7 @@ i386_sigtramp_frame_cache (struct frame_info *this_frame, void **this_cache)
}
CATCH (ex, RETURN_MASK_ERROR)
{
- if (ex.error != NOT_AVAILABLE_ERROR)
- throw_exception (ex);
+ throw_exception (ex);
}
END_CATCH