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

Pedro Alves
2013-08-05T14:12:00Z

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