This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully
- From: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: Pedro Alves <palves at redhat dot com>, Antoine Tremblay <antoine dot tremblay at ericsson dot com>, <gdb-patches at sourceware dot org>
- Date: Thu, 25 Feb 2016 08:15:17 -0500
- Subject: Re: [PATCH 1/4] Teach arm unwinders to terminate gracefully
- Authentication-results: sourceware.org; auth=none
- References: <1452188697-23870-1-git-send-email-antoine dot tremblay at ericsson dot com> <1452188697-23870-2-git-send-email-antoine dot tremblay at ericsson dot com> <86io1ung0a dot fsf at gmail dot com> <56CEE928 dot 2080704 at redhat dot com>
Pedro Alves writes:
> On 02/12/2016 02:46 PM, Yao Qi wrote:
>
>> we can wrap methods of 'struct frame_unwind' with try/catch, and handle
>> NOT_AVAILABLE_ERROR properly. In this way, each unwinder doesn't have
>> to worry about unavailable memory at all.
>>
>> Pedro, what do you think? Did you try this approach in the rest of 9
>> different ways :) (you said you "implemented this differently in about
>> 10 different ways" in your email) ?
>
> I no longer recall exactly what I tried. :-)
>
> I think it may be a good idea.
>
> 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 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.
>
> 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.)
> 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 that would fix the cases where we end up internal erroring,
> like in today's Andreas' patch:
>
> https://sourceware.org/ml/gdb-patches/2016-02/msg00773.html
>
> And then the heuristic unwinders probably no longer need to care to
> use the safe_read_memory_xxx functions.
>
> And it'd fix the bogus cases where the sentinel frame level (-1)
> shows through, due to:
>
> struct frame_info *
> get_current_frame (void)
> {
> ...
> if (current_frame == NULL)
> {
> struct frame_info *sentinel_frame =
> create_sentinel_frame (current_program_space, get_current_regcache ());
> if (catch_exceptions (current_uiout, unwind_to_current_frame,
> sentinel_frame, RETURN_MASK_ERROR) != 0)
> {
> /* Oops! Fake a current frame? Is this useful? It has a PC
> of zero, for instance. */
> current_frame = sentinel_frame;
> }
>
> See recent example here:
> https://sourceware.org/ml/gdb-patches/2016-01/msg00222.html
>
Reading Pedro's description I'm not against the refactoring but it's non
trivial to me at the moment at least.
I suggest we allow this patch to go in in order to make progress on the
arm tracepoint patchset and do that refactoring in a subsequent patch.
Would that be OK ?
Regards,
Antoine