Re: [PATCH] Only print entry values for arguments.

[Pedro Alves <palves at redhat dot com>]

On 08/05/2013 01:44 AM, Yao Qi wrote:
> Hi,
> When I think about how to handle entry values in my "skip unavailable"
> patch, I played with "set print entry-values" to different values, and
> use MI commands '-stack-list-{locals,arguments,variables}' to see the
> changes on output.  However, I find that "print entry-values" affects
> the output of "-stack-list-locals", which is not expected.
> 
> -gdb-set print entry-values only
> ^done
> -stack-list-locals --simple-values
> ^done,locals=[{name="array",type="unsigned char [2]"},{name="i@entry",type="int",value="<optimized out>"}]
> 
> -gdb-set print entry-values both
> ^done
> -stack-list-locals --simple-values
> ^done,locals=[{name="array",type="unsigned char [2]"},{name="i",type="int",value="<unavailable>"},{name="i@entry",type="int",value="<optimized out>"}]
> 
> "print entry-values" is the option about printing frame arguments,
> which has nothing to do with locals.  This patch is to check whether it
> is an argument to decide to call read_frame_arg or read_frame_local.
> read_frame_local is a new function which is to print local without
> considering much on "print entry-values".  I have to say that it is
> odd to pass "struct frame_arg *" to function read_frame_local, but it
> is required by caller and the following calls.  Functions
> list_args_or_locals and list_arg_or_local process both arguments and
> locals by means of "struct frame_arg *", so I have to use "struct
> frame_arg *" for function read_frame_local too.  Note that "print
> entry-values" is for arguments only, but is checked in both
> list_args_or_locals and list_arg_or_local.  

> We need some changes here
> to apply value of "print entry-values" to arguments only, but it is
> not the goal of this patch.

Yeah, or we could rename "struct frame_arg" to "struct frame_value"
or some such, and update it's comments to better reflect reality.


> 2013-08-05  Yao Qi  <yao@codesourcery.com>
> 
> 	* frame.h (read_frame_local): Declare.
> 	* mi/mi-cmd-stack.c (list_args_or_locals): Call
> 	read_frame_local.
> 	* stack.c (read_frame_local): New.
> 
> gdb/testsuite:
> 
> 2013-08-05  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.trace/mi-trace-unavailable.exp: Don't set
> 	"print entry-values" to "no".
> 	(test_trace_unavailable): Set various values to
> 	"print entry-values" to test the output of
> 	'-stack-list-locals' is not affected.


> diff --git a/gdb/stack.c b/gdb/stack.c
> index 510f20d..9e9ebc1 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -301,6 +301,33 @@ print_frame_arg (const struct frame_arg *arg)
>    annotate_arg_end ();
>  }
>  
> +/* Read in inferior function local SYM at FRAM into ARGP.  Caller is

typo: FRAME.

> +   responsible for xfree of ARGP->ERROR.  This function never throws an
> +   exception.  */
> +
> +void
> +read_frame_local (struct symbol *sym, struct frame_info *frame,
> +		  struct frame_arg *argp)
> +{
> +  volatile struct gdb_exception except;
> +  struct value *val = NULL;
> +  char *val_error = NULL;
> +
> +  TRY_CATCH (except, RETURN_MASK_ERROR)
> +    {
> +      val = read_var_value (sym, frame);
> +    }
> +  if (val == NULL)
> +    {
> +      val_error = alloca (strlen (except.message) + 1);
> +      strcpy (val_error, except.message);
> +    }
> +
> +  argp->sym = sym;
> +  argp->val = val;
> +  argp->error = val_error ? xstrdup (val_error) : NULL;
> +}
> +

I can see where this came from, but isn't all this the same as:

  argp->error = (val == NULL) ? xstrdup (except.message) : NULL;
  argp->val = val;
  argp->sym = sym;

?

> @@ -89,6 +86,21 @@ proc test_trace_unavailable { data_source } {
>  	    ".*\\^done,found=\"1\",tracepoint=\"${decimal}\",traceframe=\"0\",frame=\{.*" \
>  	    "-trace-find frame-number 0"
>  
> +	# Option of print entry-values shouldn't affect the output of
> +	# '-stack-list-locals'.

	# The "print entry-values" option shouldn't affect the
	# output of '-stack-list-locals'.

would read better a little less odd to me.

> -mi_gdb_test "-gdb-set print entry-values no" {\^done} \
> -    "-gdb-set print entry-values no"

...

> +	mi_gdb_test "-gdb-set print entry-values no" {\^done} ""

Was there a reason to make the message argument the empty string?

I'd rather that bit was mentioned in the ChangeLog too:

> 2013-08-05  Yao Qi  <yao@codesourcery.com>
>
> 	* gdb.trace/mi-trace-unavailable.exp: Don't set
> 	"print entry-values" to "no".
> 	(test_trace_unavailable): Set various values to
> 	"print entry-values" to test the output of
> 	'-stack-list-locals' is not affected.

 	(test_trace_unavailable): Set various values to
 	"print entry-values" to test that the output of
 	'-stack-list-locals' is not affected, and then set
	set "print entry-values" to "no".

Otherwise it looks good to me.  I wonder whether we should put
this in 7.6.1 ?

-- 
Pedro Alves