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

Pedro Alves
2014-05-29T09:41:12Z

On 05/29/2014 12:35 AM, Andrew Burgess wrote:
> On 28/05/2014 7:31 PM, Pedro Alves wrote:
>> 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?
>
> I'm happy to add the new python (and even guile) functions, I figured
> I'd post a follow up patch once these were merged ... if that's OK.

That's great, thanks.

> Commit message:
>
> Currently a MEMORY_ERROR raised during unwinding a frame will cause the
> unwind to stop with an error message, for example:
>
> (gdb) bt
> #0 breakpt () at amd64-invalid-stack-middle.c:27
> #1 0x00000000004008f0 in func5 () at amd64-invalid-stack-middle.c:32
> #2 0x0000000000400900 in func4 () at amd64-invalid-stack-middle.c:38
> #3 0x0000000000400910 in func3 () at amd64-invalid-stack-middle.c:44
> #4 0x0000000000400928 in func2 () at amd64-invalid-stack-middle.c:50
> Cannot access memory at address 0x2aaaaaab0000
>
> However, frame #4 is marked as being the end of the stack unwind, so a
> subsequent request for the backtrace looses the error message, such as:
>
> (gdb) bt
> #0 breakpt () at amd64-invalid-stack-middle.c:27
> #1 0x00000000004008f0 in func5 () at amd64-invalid-stack-middle.c:32
> #2 0x0000000000400900 in func4 () at amd64-invalid-stack-middle.c:38
> #3 0x0000000000400910 in func3 () at amd64-invalid-stack-middle.c:44
> #4 0x0000000000400928 in func2 () at amd64-invalid-stack-middle.c:50
>
> When fetching the backtrace, or requesting the stack depth using the MI
> interface the situation is even worse, the first time a request is made
> we encounter the memory error and so the MI returns an error instead of
> the correct result, for example:
>
> (gdb) -stack-info-depth
> ^error,msg="Cannot access memory at address 0x2aaaaaab0000"
>
> Or,
>
> (gdb) -stack-list-frames
> ^error,msg="Cannot access memory at address 0x2aaaaaab0000"
>
> However, once one of these commands has been used gdb has, internally,
> walked the stack and figured that out that frame #4 is the bottom of the
> stack, so the second time an MI command is tried you'll get the "expected"
> result:
>
> (gdb) -stack-info-depth
> ^done,depth="5"
>
> Or,
>
> (gdb) -stack-list-frames
> ^done,stack=[frame={level="0", .. snip lots .. }]

Now here's an excellent description / commit message. Thanks!

>
> After this patch the MEMORY_ERROR encountered during the frame unwind is
> attached to frame #4 as the stop reason, and is displayed in the CLI each
> time the backtrace is requested.

Please show examples of this in the commit log too.

> In the MI, catching the error means that
> the "expected" result is returned the first time the MI command is issued.

> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -3199,6 +3199,9 @@ stack corruption.
> The frame unwinder did not find any saved PC, but we needed
> one to unwind further.
>
> +@item gdb.FRAME_UNWIND_MEMORY_ERROR
> +The frame unwinder caused an error while trying to access memory.

> +
> + /* The error needs to live as long as the frame does. */
> + stop_string =
> + FRAME_OBSTACK_CALLOC (strlen (ex.message) + 1, char);

#1 - Per GNU coding conventions, operators go on the next line, and '='
is an operator (assignment), so:

stop_string
= FRAME_OBSTACK_CALLOC (strlen (ex.message) + 1, char);

#2 - sizeof (char) is 1 by definition, so no need for calloc, actually.
And then that might fit on a single line:

stop_string = FRAME_OBSTACK_ZALLOC (strlen (ex.message) + 1);

> + strcpy (stop_string, ex.message);
> + this_frame->stop_string = stop_string;

But even better we could do this instead:

size_t size;

size = strlen (ex.message) + 1;
this_frame->stop_string = FRAME_OBSTACK_ZALLOC (size);
memcpy (this_frame->stop_string, ex.message, size);

> + }
> + prev_frame = NULL;
> + }
> + else
> + throw_exception (ex);
> + }
> +
> + return prev_frame;
> +}
> +
> /* Construct a new "struct frame_info" and link it previous to
> this_frame. */
>
> @@ -2576,6 +2618,24 @@ unwind_frame_stop_reason_string (enum unwind_stop_reason 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 *);

Let's include an example to make it clearer:

/* Return a possibly frame specific string explaining why the unwind
stopped here. E.g., if unwinding tripped on a memory error, this
will return the error description string, which includes the address
that we failed to access. If there's no specific reason stored for
a frame then a generic reason string will be returned.

Should only be called for frames that don't have a previous frame. */

(I imagine that this "Should" will need to be addressed when we expose
this to Python somehow. We wouldn't want failure to comply to that
to cause an internal error.)

> +/* Return a possibly frame specific string explaining why the unwind
> + stopped at frame FI. Must only be called if there is no previous
> + frame. */

Let's just remove this comment, thus avoiding maintaining the same
description in two places. Note it was already divergent from the
comment in the header.

> +
> +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;

if (fi->stop_string != NULL)
return fi->stop_string;

Otherwise looks good.

Thanks,
Pedro Alves