Re: [PATCH] -stack-info-depth should always return a count.

[Keith Seitz <keiths at redhat dot com>]

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