Re: [PATCH] Trace state variables

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Tue, 15 Dec 2009 05:51:18 -0800
> From: Stan Shebs <stan@codesourcery.com>
> 
> One of our tracepoint enhancements is to add the notion of a "trace 
> state variable", which is a GDB-defined piece of data managed by the 
> tracepoint agent on the target; one could think of it as a target-side 
> convenience variable.

Thanks.

> In keeping with their convenience-like nature, they are introduced
> with a dollar sign

But the code seems to allow with or without the $, right?  Like here:

> +   /* Be relaxed about the special character.  */
> +   if (*name == '$')
> +     ++name;

A few comments:

> +   /* Make sure the tsv number is in range.  */
> +   if (num < 0 || num > 0xffff)
> +     error (_("GDB bug: ax-general.c (ax_tsv): variable number out of range"));

Should this be internal_error instead?

> +   warning (_("No trace variable named \"$%s\", not deleting"), name);

The "not deleting" part seems unnecessary.

> + 	printf_filtered ("  <unknown>");
> +       else
> + 	/* It is not meaningful to ask about the value.  */
> + 	printf_filtered ("  <undefined>");

No translations for these two?

> Index: doc/agentexpr.texi
> ===================================================================
> RCS file: /cvs/src/src/gdb/doc/agentexpr.texi,v
> retrieving revision 1.9
> diff -p -r1.9 agentexpr.texi
> *** doc/agentexpr.texi	11 Nov 2009 15:08:50 -0000	1.9
> --- doc/agentexpr.texi	15 Dec 2009 13:28:33 -0000

This part is fine.

> + @node Trace State Variables
> + @subsection Trace State Variables
> + 
> + A @dfn{trace state variable} is a special type of variable that is
> + created and managed by target-side code. The syntax is the same as

This introduces a new terminology, so please add an index entry for it.

> + but they are stored on the target. They must be created explicitly,
                                     ^^
Two spaces between sentences, please (here and elsewhere).

> + while the trace experiment is running.  Trace state variables share
> + the same namespace as other ``$'' variables, which means that you
> + cannot have trace state variables with names like @code{$23} or
> + @code{$pc}, nor can you have a trace state variable and a convenience
> + variable with the same name.

I would suggest here an index entry like

  @cindex convenience variables, and trace state variables

> + @item tvariable $@var{name} [ = @var{expression} ]
> + @kindex tvariable
> + @cindex trace state variable

This @cindex entry should be moved to the beginning of the subsection,
and will then serve as the indexing of the term "trace state variable"
that this section introduces.

> + The @code{tvariable} command creates a new trace state variable named
> + @code{$}@var{name}
    ^^^^^^^^^^^^^^^^^^
@code{$@var{name}}

> +                                           A second @code{tvariable}
> + command with the same name assigns a new value to the variable.

I don't understand this sentence: what ``second command''? what ``new
value''?  Did you mean to say that each tvariable command overwrites
the previous value by a new one?

> + @item info tvariables
> + @kindex tvariables

We already have "@kindex tvariable" a few lines above, so this @kindex
is not useful.  Suggest to remove it, or replace with

 @kindex info tvariables

> + List all the trace state variables along with their initial values.
                                                  ^^^^^^^^^^^^^^^^^^^^
But in reality the current values are displayed as well, right?

> + @item QTDV:@var{n}:@var{value}

Don't we have index entries for all the packets we support?

> + @item V@var{value}
> + The value of the variable is @var{value}.  This will be the current
> + value of the variable if the user is examining a running target, or a
> + saved value if the variable was collected in the trace frame that the
> + user is looking at.  Note that multiple requests may result in different
> + reply values, such as for variables like @code{$trace_timestamp} that are
> + computed by the target program.

What is this $trace_timestamp variable mentioned in the last sentence?
I thought trace state variables need to be declared before they can be
used, so how come we evidently have special internally generated
variables with magic features?  What am I missing here?