Re: [PATCH] Trace state variables

[Stan Shebs <stan@codesourcery.com>]

Eli Zaretskii wrote:
>> 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;
>>     
I was thinking to let internal API work or without, for flexibility, but 
this routine is only called in one place, so I'll just make it 
consistently require.
>
> 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?
>   
Yep.
>   
>> +   warning (_("No trace variable named \"$%s\", not deleting"), name);
>>     
>
> The "not deleting" part seems unnecessary.
>   
I suppose so.  I always like to have warnings say what GDB is going to 
do with the situation, even if it seems somewhat obvious to us, because 
it may not be as obvious to the user (after all, they're getting a 
warning because things are not going as expected).
>> + 	printf_filtered ("  <unknown>");
>> +       else
>> + 	/* It is not meaningful to ask about the value.  */
>> + 	printf_filtered ("  <undefined>");
>>     
>
> No translations for these two?
>
>   
Oversight.
>> +                                           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?
>   
Yeah, I'll improve the phrasing.
>> + List all the trace state variables along with their initial values.
>>     
>                                                   ^^^^^^^^^^^^^^^^^^^^
> But in reality the current values are displayed as well, right?
>   
Yep.
>> + @item QTDV:@var{n}:@var{value}
>>     
>
> Don't we have index entries for all the packets we support?
>   
Excellent idea, I should do that. :-)
>> + @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?
>
>   
Oops, a communique from the future. :-) An upcoming patch will add the 
capability for a target to define its own builtin
variables, whose definitions are uploaded upon connection.

Stan