[PATCH] Print trace state variables

[PATCH] Print trace state variables

[Stan Shebs]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

This patch remedies an oversight in recent tracepoint work, by adding 
printing of trace state variables.  While you can see tsv values using 
"info tvariables", it's much more useful to be able to include them in 
print commands and expressions in general; you can then display them in 
different formats, or compute the difference between a tsv and some 
other value.  The manual already documents this as working, only the 
code to do it has been missing. :-)

This has been in CodeSourcery's version for some time, but I set it 
aside for awhile because it seemed a little kludgy to add a 
tracepoint-specific case into general evaluation.  On the plus side, its 
effect is localized, and should be safe for a 7.2 update release, where 
it will be useful for work on trace support in Eclipse.  There are no 
regressions testing with either i386-linux native or GDBserver.

If this seems reasonable, I'll put in both trunk and 7.2 branch.

Stan

2010-12-08  Stan Shebs <stan@codesourcery.com>

     * value.c (value_of_internalvar): Add case for trace state
     variables.

     * gdb.trace/tsv.exp: Test print command on trace state variables.




[-- Attachment #2: ptsv-patch-1 --]
[-- Type: text/plain, Size: 4504 bytes --]

Index: value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.118
diff -p -r1.118 value.c
*** value.c	29 Nov 2010 21:18:16 -0000	1.118
--- value.c	9 Dec 2010 01:03:40 -0000
***************
*** 42,47 ****
--- 42,49 ----
  
  #include "python/python.h"
  
+ #include "tracepoint.h"
+ 
  /* Prototypes for exported functions. */
  
  void _initialize_values (void);
*************** struct value *
*** 1197,1202 ****
--- 1199,1220 ----
  value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var)
  {
    struct value *val;
+   struct trace_state_variable *tsv;
+ 
+   /* If there is a trace state variable of the same name, assume that
+      is what we really want to see.  */
+   tsv = find_trace_state_variable (var->name);
+   if (tsv)
+     {
+       tsv->value_known = target_get_trace_state_variable_value (tsv->number,
+ 								&(tsv->value));
+       if (tsv->value_known)
+ 	val = value_from_longest (builtin_type (gdbarch)->builtin_int64,
+ 				  tsv->value);
+       else
+ 	val = allocate_value (builtin_type (gdbarch)->builtin_void);
+       return val;
+     }
  
    switch (var->kind)
      {
Index: testsuite/gdb.trace/tsv.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/tsv.exp,v
retrieving revision 1.5
diff -p -r1.5 tsv.exp
*** testsuite/gdb.trace/tsv.exp	2 Jun 2010 21:55:28 -0000	1.5
--- testsuite/gdb.trace/tsv.exp	9 Dec 2010 01:03:40 -0000
*************** set srcfile ${testfile}.c
*** 27,41 ****
  set binfile $objdir/$subdir/tsv
  if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
  	  executable {debug nowarnings}] != "" } {
!     untested tracecmd.exp
      return -1
  }
! gdb_reinitialize_dir $srcdir/$subdir
! 
! # If testing on a remote host, download the source file.
! # remote_download host $srcdir/$subdir/$srcfile
! 
! gdb_file_cmd $binfile
  
  gdb_test "tvariable \$tvar1" \
    "Trace state variable \\\$tvar1 created, with initial value 0." \
--- 27,36 ----
  set binfile $objdir/$subdir/tsv
  if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
  	  executable {debug nowarnings}] != "" } {
!     untested tsv.exp
      return -1
  }
! gdb_load $binfile
  
  gdb_test "tvariable \$tvar1" \
    "Trace state variable \\\$tvar1 created, with initial value 0." \
*************** gdb_test "info tvariables" \
*** 72,77 ****
--- 67,75 ----
  \\\$tvar3\[\t \]+1234567000000\[\t \]+.*<undefined>.*" \
    "List tvariables"
  
+ gdb_test "print \$tvar2" " = void" \
+     "Print a trace state variable before run"
+ 
  gdb_test_no_output "delete tvariable \$tvar2" \
    "delete trace state variable"
  
*************** gdb_test "info tvariables" \
*** 91,94 ****
--- 89,150 ----
    "No trace state variables.*" \
    "List tvariables after deleting all"
  
+ # Now try running a trace.
+ 
+ runto_main
+ gdb_reinitialize_dir $srcdir/$subdir
+ 
+ # The rest of the testing needs actual tracing to work.
+ if { ![gdb_target_supports_trace] } then {
+     pass "Current target does not support trace"
+     return 1;
+ }
+ 
+ # define relative source line numbers:
+ # all subsequent line numbers are relative to this first one (baseline)
+ 
+ set baseline  [gdb_find_recursion_test_baseline $srcfile];
+ if { $baseline == -1 } then {
+     fail "Could not find gdb_recursion_test function"
+     return;
+ }
+ 
+ set testline1 [expr $baseline + 7]
+ 
+ gdb_delete_tracepoints
+ set trcpt1 [gdb_gettpnum gdb_c_test];
+ set trcpt2 [gdb_gettpnum gdb_asm_test];
+ set trcpt3 [gdb_gettpnum $testline1];
+ if { $trcpt1 <= 0 || $trcpt2 <= 0 || $trcpt3 <= 0 } then {
+     fail "setting tracepoints"
+     return;
+ }
+ 
+ gdb_test "tvariable \$tvar5 = 15" \
+   "Trace state variable \\\$tvar5 created, with initial value 15." \
+   "Create a trace state variable tvar5"
+ 
+ gdb_trace_setactions "collect tsv for first tracepoint" \
+ 	"$trcpt1" \
+ 	"collect \$tvar5 += 1" "^$"
+ 
+ gdb_test "tstart" ".*" ""
+ 
+ gdb_test "print \$tvar5" " = 15" \
+     "Print a trace state variable at start of run"
+ 
+ # Be sure not to fall off the end of the program.
+ gdb_test "break end" ".*" ""
+ gdb_test "continue" \
+     "Continuing.*Breakpoint $decimal, end.*" \
+     "run trace experiment"
+ 
+ gdb_test "print \$tvar5" " = 16" \
+     "Print a trace state variable during run"
+ 
+ gdb_test "tstop" ".*" ""
+ 
+ gdb_test "print \$tvar5" " = 16" \
+     "Print a trace state variable after run"
+ 
  

Re: [PATCH] Print trace state variables

[Joel Brobecker]

> If this seems reasonable, I'll put in both trunk and 7.2 branch.

Is this the change that you were thinking we should wait for before
releasing 7.2.1?

Tristan just informed me yesterday of another issue with etc/gnu-oids.texi
missing, and I will backport his change in a second.  If all is ready and
in, I should be able to make the release on, say, Monday.

-- 
Joel

Re: [PATCH] Print trace state variables

[Stan Shebs]

On 12/10/10 3:26 AM, Joel Brobecker wrote:
>> If this seems reasonable, I'll put in both trunk and 7.2 branch.
> Is this the change that you were thinking we should wait for before
> releasing 7.2.1?

Yep.

Stan

RE: [PATCH] Print trace state variables

[Marc Khouzam]

From: Joel Brobecker [brobecker@adacore.com]

> > If this seems reasonable, I'll put in both trunk and 7.2 branch.
> 
> Is this the change that you were thinking we should wait for before
> releasing 7.2.1?
> 
> Tristan just informed me yesterday of another issue with etc/gnu-oids.texi
> missing, and I will backport his change in a second.  If all is ready and
> in, I should be able to make the release on, say, Monday.

Hi,

I was hoping to get the patch about the assertion failure when removing
an inferior into 7_2.  I believe Tom was ok with my solution, I'm just 
waiting for final approval.

http://sourceware.org/ml/gdb-patches/2010-12/msg00085.html

We actually hit this problem in Eclipse and GDB crashes when the user
tries to detach from a running process.  We could work around it if we
have to, but since I'm close to a fix for GDB, I was hoping to use
that solution with 7.2.1

Thanks

Marc

Re: [PATCH] Print trace state variables

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> I was hoping to get the patch about the assertion failure when removing
Marc> an inferior into 7_2.  I believe Tom was ok with my solution, I'm just 
Marc> waiting for final approval.

I went ahead and approved it.
I hope I am not stepping on any toes.
It seemed reasonable to me and nobody else commented on it.

Tom

Re: [PATCH] Print trace state variables

[Joel Brobecker]

> Marc> I was hoping to get the patch about the assertion failure when removing
> Marc> an inferior into 7_2.  I believe Tom was ok with my solution, I'm just 
> Marc> waiting for final approval.
> 
> I went ahead and approved it.
> I hope I am not stepping on any toes.
> It seemed reasonable to me and nobody else commented on it.

Excellent! I think this makes everyone happy :). Thanks, Tom.

-- 
Joel

Re: [PATCH] Print trace state variables

[Joel Brobecker]

> This has been in CodeSourcery's version for some time, but I set it
> aside for awhile because it seemed a little kludgy to add a
> tracepoint-specific case into general evaluation.

I think a cleaner way of doing this would be to create a new OP_ enum
for tracepoint variables.  We'd then add handling for it in
write_dollar_variable, as well as in the expression evaluator.

One potential issue with that approach is that it might require each
language to also add handling for that operator, but if all languages
are implemented the way Ada is (for operators that do not need to be
handled specifically in Ada, we default to the standard evaluator
(evaluate_subexp_standard or something like this).

Another potential issue to consider is precedence: If the user had
already defined an internal variable called "VAR", and then creates
a tracepoint variable with the same name, which one should we print
when he write "$VAR"? With your proposal, the tracepoint variable
hides the internal variable, right?

One question that worries me: What if the user tries to assign to
an internal variable which collides with one of the tracepoint
variables, and then tries to print it? I think he'll see the tracepoint
variable value, which will make it look like as if his assignment
had no effect.

> 2010-12-08  Stan Shebs <stan@codesourcery.com>
> 
>     * value.c (value_of_internalvar): Add case for trace state
>     variables.
> 
>     * gdb.trace/tsv.exp: Test print command on trace state variables.

No objection to this, though, since it is relatively contained.

-- 
Joel

Re: [PATCH] Print trace state variables

[Stan Shebs]

On 12/10/10 9:55 PM, Joel Brobecker wrote:
>> This has been in CodeSourcery's version for some time, but I set it
>> aside for awhile because it seemed a little kludgy to add a
>> tracepoint-specific case into general evaluation.
> I think a cleaner way of doing this would be to create a new OP_ enum
> for tracepoint variables.  We'd then add handling for it in
> write_dollar_variable, as well as in the expression evaluator.
>
> One potential issue with that approach is that it might require each
> language to also add handling for that operator, but if all languages
> are implemented the way Ada is (for operators that do not need to be
> handled specifically in Ada, we default to the standard evaluator
> (evaluate_subexp_standard or something like this).

I originally thought adding an OP was too intrusive for what is kind of 
a special case, but I suppose it's not really any freakier than some of 
the other expression types. :-)

> Another potential issue to consider is precedence: If the user had
> already defined an internal variable called "VAR", and then creates
> a tracepoint variable with the same name, which one should we print
> when he write "$VAR"? With your proposal, the tracepoint variable
> hides the internal variable, right?

That's right.  My original specification called for the variables to use 
$-syntax despite the overloading, because it generally fits into user 
expectation as a variable that is not part of the program, and also 
there are not a lot of alternatives available in parsing.  The manual 
dictates that the two types of variables cannot share a name, so a followon
patch could detect and disallow overloading - although it seems hard to 
make it user-friendly, since both kinds of variables can appear in 
scripts that are loaded.

>> 2010-12-08  Stan Shebs<stan@codesourcery.com>
>>
>>      * value.c (value_of_internalvar): Add case for trace state
>>      variables.
>>
>>      * gdb.trace/tsv.exp: Test print command on trace state variables.
> No objection to this, though, since it is relatively contained.
>

Thanks, I put this in.

I do hope to re-address this soon, as part of a project to do 
target-side conditional breakpoints (read: exploit the tracepoint agent 
outside of tracing), and make trace state variables into target-managed 
variables in general.

Stan

Re: [PATCH] Print trace state variables

[Hui Zhu]

Cool!

This function is really great!

Thanks,
Hui

On Fri, Dec 10, 2010 at 06:38, Stan Shebs <stan@codesourcery.com> wrote:
> This patch remedies an oversight in recent tracepoint work, by adding
> printing of trace state variables.  While you can see tsv values using "info
> tvariables", it's much more useful to be able to include them in print
> commands and expressions in general; you can then display them in different
> formats, or compute the difference between a tsv and some other value.  The
> manual already documents this as working, only the code to do it has been
> missing. :-)
>
> This has been in CodeSourcery's version for some time, but I set it aside
> for awhile because it seemed a little kludgy to add a tracepoint-specific
> case into general evaluation.  On the plus side, its effect is localized,
> and should be safe for a 7.2 update release, where it will be useful for
> work on trace support in Eclipse.  There are no regressions testing with
> either i386-linux native or GDBserver.
>
> If this seems reasonable, I'll put in both trunk and 7.2 branch.
>
> Stan
>
> 2010-12-08  Stan Shebs <stan@codesourcery.com>
>
>    * value.c (value_of_internalvar): Add case for trace state
>    variables.
>
>    * gdb.trace/tsv.exp: Test print command on trace state variables.
>
>
>
>

Re: [PATCH] Print trace state variables

[Pedro Alves]

On Saturday 11 December 2010 05:55:10, Joel Brobecker wrote:
> Another potential issue to consider is precedence: If the user had
> already defined an internal variable called "VAR", and then creates
> a tracepoint variable with the same name, which one should we print
> when he write "$VAR"? With your proposal, the tracepoint variable
> hides the internal variable, right?

For the archives, I'd like to point out explicitly that 
there's some precedence for this with registers:

>./gdb 
(gdb) p $rax
$1 = void
(gdb) p $rax = 1
$2 = 1
(gdb) file gdb
Reading symbols from /home/pedro/gdb/baseline/build/gdb/gdb...done.
(gdb) p $rax 
No registers.
(gdb) start
Temporary breakpoint 1 at 0x4562f3: file ../../src/gdb/gdb.c, line 29.
Starting program: /home/pedro/gdb/baseline/build/gdb/gdb 
[Thread debugging using libthread_db enabled]

Temporary breakpoint 1, main (argc=
During symbol reading, incomplete CFI data; unspecified registers (e.g., rax) at 0x4562e8.
1, argv=0x7fffffffe108) at ../../src/gdb/gdb.c:29
29        memset (&args, 0, sizeof args);
(gdb) p $rax 
$3 = 12108976

-- 
Pedro Alves

Re: [PATCH] Print trace state variables

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I think a cleaner way of doing this would be to create a new OP_ enum
Joel> for tracepoint variables.  We'd then add handling for it in
Joel> write_dollar_variable, as well as in the expression evaluator.

FWIW, I mildly prefer the current approach.  Using a new OP_ means that
the variable is fixed at expression-parse time; but there doesn't seem
to be a compelling need to make this limitation.  In fact, it seems like
it could be confusing... breakpoint conditions entered at different
times would refer to different variables (some hidden!), or re-parsing
an expression might resolve the variable differently.

Joel> Another potential issue to consider is precedence: If the user had
Joel> already defined an internal variable called "VAR", and then creates
Joel> a tracepoint variable with the same name, which one should we print
Joel> when he write "$VAR"? With your proposal, the tracepoint variable
Joel> hides the internal variable, right?

The docs say:

    Trace state variables share the same
    namespace as other "$" variables, which means that you cannot have
    trace state variables with names like `$23' or `$pc', nor can you have
    a trace state variable and a convenience variable with the same name.

Maybe this could be enforced?  Or is that not possible?

Tom