-stack-info-depth should always return a count.

  The MI command -stack-info-depth return the "depth" field in most
cases.  However, if due to stack corruption, gdb tries to access
some invalid memory then the result is an error and no depth.

I believe that the current behaviour would be better if gdb always
returned a depth number, indicating the number of frames that are
available before the stack corruption, the patch below does this.

I have also added an new "reason" field to the -stack-info-depth
reply, the reason is a string that tries to  explain why gdb is
giving this depth as an answer.

For stacks without any corruption the only two possible reason
strings are "All frames" (this is the total stack depth), or
"Reached frame limit" (a LIMIT was passed to -stack-info-depth
and reached).  If there is stack corruption then the reason will
be the error string from gdb.

OK to apply?

Thanks,
Andrew

gdb/ChangeLog:

2014-03-27  Andrew Burgess  <aburgess@broadcom.com>

	* mi/mi-cmd-stack.c (mi_cmd_stack_info_depth): Add TRY_CATCH
	around the stack unwind.  Add a reason field to explain the given
	depth.
	* NEWS: Mention changes to -stack-info-depth command.

gdb/testsuite/ChangeLog:

2014-03-27  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.mi/mi-stack.exp (test_stack_info_depth): Expect new reason
	field.
gdb/doc/ChangeLog:

2014-03-27  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.texinfo (gdb/mi Stack Manipulation): Add information and
	examples explaining new reason field for -stack-info-depth
	command.
  

> Date: Thu, 27 Mar 2014 13:20:25 +0000
> From: Andrew Burgess <aburgess@broadcom.com>
> 
> +* MI changes
> +
> +  ** The -stack-info-depth command will now always return a depth even if
> +     stack corruption is encountered.  A new reason string is also returned
> +     that explains why gdb is reporting this depth.
                          ^^^
"GDB", in caps.

> +Return the depth of the stack.  If the integer argument
> +@var{max-depth} is specified, do not count beyond @var{max-depth}
> +frames.  The reason why gdb returns the answer it does is also given.
                           ^^^
@value{GDBN}

> +Possible reasons are @var{All frames}, @var{Reached frame limit}, or
> +an error message.

@var is incorrect here, please use @samp instead.

> +An example where there are 3 valid stack frames, after which the stack
> +pointer is corrupted and gdb is unable to unwind the stack further:
                            ^^^
@value{GDBN}

The documentation parts are OK with those changes.

Thanks.
  

Hi,

Thank you for the patch. A few comments below.

On 03/27/2014 06:20 AM, Andrew Burgess wrote:

> I believe that the current behaviour would be better if gdb always
> returned a depth number, indicating the number of frames that are
> available before the stack corruption, the patch below does this.

Out of curiosity, how does -stack-list-frames behave in this scenario? 
IMO both these commands should offer complimentary data, i.e., if 
-stack-list-frames outputs N frames before an error, then 
-stack-info-depth should return N. If -stack-list-frames outputs no 
frames, then -stack-info-depth should return zero.

If, as I suspect, -stack-list-frames and -stack-info-depth output 
different numbers of frames for this situation, then I agree. This patch 
is needed.

If not, well, I don't see any harm, but I have no idea if this kind of 
change might cause problems for current MI consumers. [To be honest, I'm 
not sure what value -stack-info-depth has to begin with.] A global/MI 
maintainer will have to give a recommendation in that case.

> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index 7bc8114..9a4eec7 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -170,8 +170,10 @@ void
>   mi_cmd_stack_info_depth (char *command, char **argv, int argc)
>   {
>     int frame_high;
> -  int i;
> +  int i = 0;
>     struct frame_info *fi;
> +  volatile struct gdb_exception except;
> +  const char *reason;
>
>     if (argc > 1)
>       error (_("-stack-info-depth: Usage: [MAX_DEPTH]"));
> @@ -183,12 +185,29 @@ mi_cmd_stack_info_depth (char *command, char **argv, int argc)
>          the stack.  */
>       frame_high = -1;
>
> -  for (i = 0, fi = get_current_frame ();
> -       fi && (i < frame_high || frame_high == -1);
> -       i++, fi = get_prev_frame (fi))
> -    QUIT;
> +  TRY_CATCH (except, RETURN_MASK_ERROR)
> +    {
> +      for (i = 0, fi = get_current_frame ();
> +	   fi && (i < frame_high || frame_high == -1);
> +	   i++, fi = get_prev_frame (fi))
> +	QUIT;
> +    }
> +
> +  if (except.message)

Maintainers recently agreed to do explicit checks for NULL. [if 
(except.message != NULL) ...]

> +    {
> +      /* Due to the lazy way state is fetched from the target, if we have
> +	 an exception our depth count will be one greater than it should
> +	 be.  */
> +      reason = except.message;
> +      --i;
> +    }
> +  else if (frame_high == -1 || i < frame_high)
> +    reason = _("All frames");
> +  else
> +    reason = _("Reached frame limit");
>
>     ui_out_field_int (current_uiout, "depth", i);
> +  ui_out_field_string (current_uiout, "reason", reason);
>   }
>
>   /* Print a list of the locals for the current frame.  With argument of
>   2014-03-26  Yao Qi  <yao@codesourcery.com>
>
>   	* lib/gdb.exp (readline_is_used): New proc.

I presume this is a cut-n-paste-o...

> diff --git a/gdb/testsuite/gdb.mi/mi-stack.exp b/gdb/testsuite/gdb.mi/mi-stack.exp
> index ac7db7a..4977342 100644
> --- a/gdb/testsuite/gdb.mi/mi-stack.exp
> +++ b/gdb/testsuite/gdb.mi/mi-stack.exp
> @@ -137,15 +137,15 @@ proc test_stack_info_depth {} {
>       # -stack-info-depth 99
>
>       mi_gdb_test "231-stack-info-depth" \
> -	    "231\\^done,depth=\"5\"" \
> +        "231\\^done,depth=\"5\",reason=\"\[^\"\]+\"" \
>                   "stack info-depth"
>
>       mi_gdb_test "231-stack-info-depth 3" \
> -	    "231\\^done,depth=\"3\"" \
> +	    "231\\^done,depth=\"3\",reason=\"\[^\"\]+\"" \
>                   "stack info-depth 3"
>
>       mi_gdb_test "231-stack-info-depth 99" \
> -	    "231\\^done,depth=\"5\"" \
> +	    "231\\^done,depth=\"5\",reason=\"\[^\"\]+\"" \
>                   "stack info-depth 99"
>
>       mi_gdb_test "231-stack-info-depth 99 99" \
>

Is there any reason to omit the actual reason="..." in these tests? I 
don't see where the reason="..." is tested otherwise. I think this 
necessary. I'm a ever-growing fan of coverage testing.

Is there any way to reliably test the error case/reason="<some error 
message>"?

Keith
  

>>>>> "Andrew" == Andrew Burgess <aburgess@broadcom.com> writes:

Andrew> I have also added an new "reason" field to the -stack-info-depth
Andrew> reply, the reason is a string that tries to  explain why gdb is
Andrew> giving this depth as an answer.

Andrew> OK to apply?

It seems like a reasonable approach to me.

I wonder whether any MI users are likely to have issues.  It would be
nice if we had better communication channels here :( Anyway, have you
looked at any of them?  Or, does a particular program motivate this
patch?

I didn't see any answer to Keith's questions.

Tom
  

On 03/27/2014 05:20 PM, Andrew Burgess wrote:

> -^done,depth="12"
> +^done,depth="12",reason="All frames"
>   (gdb)
>   -stack-info-depth 4
> -^done,depth="4"
> +^done,depth="4",reason="Reached frame limit"
>   (gdb)
>   -stack-info-depth 12
> -^done,depth="12"
> +^done,depth="12",reason="Reached frame limit"
>   (gdb)
>   -stack-info-depth 11
> -^done,depth="11"
> +^done,depth="11",reason="Reached frame limit"
>   (gdb)
>   -stack-info-depth 13
> -^done,depth="12"
> +^done,depth="12",reason="All frames"
> +(gdb)
> +@end smallexample
> +
> +An example where there are 3 valid stack frames, after which the stack
> +pointer is corrupted and gdb is unable to unwind the stack further:
> +
> +@smallexample
> +(gdb)
> +-stack-info-depth
> +^done,depth="3",reason="Cannot access memory at address 0xffffffff"
> +(gdb)
> +-stack-info-depth 4
> +^done,depth="3",reason="Cannot access memory at address 0xffffffff"
> +(gdb)
> +-stack-info-depth 3
> +^done,depth="3",reason="Reached frame limit"
>   (gdb)
>   @end smallexample

I think using human readable reasons like is a bit inconvenient, given that the consumer is
a computer. Can we use has_more field, like we do somewhere else, to indicate that more
frames might be available in case of specified depth limit? And use 'error' field in case
we run into actual error (in which case, has_more field will not be provided)?

Thanks,
Volodya