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

[Pedro Alves <palves@redhat.com>]

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