[3/5,v4] Fix argument to compiled_cond, and add cases for compiled-condition.
Commit Message
Hi,
Ulrich Weigand wrote:
> Ah, when I said to add new test cases in a separate patch, what I meant was:
> - use a separate patch (applied *first*) that adds the *new tests* (to be
> run on existing platforms), i.e. test_ftrace_condition
> - as part of the patch that actually adds powerpc support, add all the small
> test case snippets that specifically enable the test cases for powerpc
> This is again so that each set in a series is meaningful in itself (and
> does not introduce testsuite regressions when applied alone).
...
> Wei-cheng Wang wrote:
> >if (tpoint->compiled_cond)
> > err = ((condfn) (uintptr_t) (tpoint->compiled_cond)) (ctx, &value);
> >
> >I think probably either we could pass ctx->regs to compiled_cond instead,
> >or move the declarations of fast_tracepoint_ctx (and others) to tracepoint.h,
> >so we can use "offsetof (fast_tracepoint_ctx, regs)" instead.
> >Any suggestion?
> FWIW, passing the regs buffer directly to the compiled routine seems
> more straightforward to me ...
Some of the new cases are used to testing emit-reg, and emit-reg for x86
doesn't work due to the incorrect argument to compiled_cond - "regs" buffer
is expected, but tracepoint context is passed
This case also includes the fix for complied_cond in order for x86 to
pass testing for emit-reg op.
Testing result on my x86_64 Ubuntu 14.04.2 TLS
before w/ new cases only w/ the fix
PASS 2625 2759 2765
FAIL 33 39 33
XFAIL 16 16 16
KFAIL 2 2 2
UNTESTED 1 1 1
UNSUPPORTED 3 3 3
Thanks,
Wei-cheng
---
* Fix generating emit-reg op by passing register buffer to compiled_cond.
* Add a new function for testing compiled-cond by checking whether
based on a given CONDEXP, a list of expected values should be collected.
---
gdb/gdbserver/ChangeLog
2015-06-27 Wei-cheng Wang <cole945@gmail.com>
* tracepoint.c (eval_result_type): Change prototype.
(condition_true_at_tracepoint): Fix argument to compiled_cond.
gdb/testsuite/ChangeLog
2015-06-27 Wei-cheng Wang <cole945@gmail.com>
* gdb.trace/ftrace.exp: (test_ftrace_condition) New function
for testing bytecode compilation.
---
gdb/gdbserver/tracepoint.c | 7 +++--
gdb/testsuite/gdb.trace/ftrace.exp | 64 ++++++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+), 2 deletions(-)
Comments
On 06/27/2015 05:21 PM, Wei-cheng Wang wrote:
> Hi,
>
> Ulrich Weigand wrote:
>> > Ah, when I said to add new test cases in a separate patch, what I meant was:
>> > - use a separate patch (applied *first*) that adds the *new tests* (to be
>> > run on existing platforms), i.e. test_ftrace_condition
>> > - as part of the patch that actually adds powerpc support, add all the small
>> > test case snippets that specifically enable the test cases for powerpc
>> > This is again so that each set in a series is meaningful in itself (and
>> > does not introduce testsuite regressions when applied alone).
> ...
>> > Wei-cheng Wang wrote:
>>> > >if (tpoint->compiled_cond)
>>> > > err = ((condfn) (uintptr_t) (tpoint->compiled_cond)) (ctx, &value);
>>> > >
>>> > >I think probably either we could pass ctx->regs to compiled_cond instead,
>>> > >or move the declarations of fast_tracepoint_ctx (and others) to tracepoint.h,
>>> > >so we can use "offsetof (fast_tracepoint_ctx, regs)" instead.
>>> > >Any suggestion?
>> > FWIW, passing the regs buffer directly to the compiled routine seems
>> > more straightforward to me ...
> Some of the new cases are used to testing emit-reg, and emit-reg for x86
> doesn't work due to the incorrect argument to compiled_cond - "regs" buffer
> is expected, but tracepoint context is passed
>
> This case also includes the fix for complied_cond in order for x86 to
> pass testing for emit-reg op.
>
> Testing result on my x86_64 Ubuntu 14.04.2 TLS
>
> before w/ new cases only w/ the fix
> PASS 2625 2759 2765
> FAIL 33 39 33
> XFAIL 16 16 16
> KFAIL 2 2 2
> UNTESTED 1 1 1
> UNSUPPORTED 3 3 3
>
> Thanks,
> Wei-cheng
>
> ---
>
> * Fix generating emit-reg op by passing register buffer to compiled_cond.
> * Add a new function for testing compiled-cond by checking whether
> based on a given CONDEXP, a list of expected values should be collected.
>
> ---
FWIW, looks good to me too. Thanks for the new tests!
Thanks,
Pedro Alves
Hi Wei-cheng,
On 27/06/15 17:21, Wei-cheng Wang wrote:
> Hi,
>
> Ulrich Weigand wrote:
>> Ah, when I said to add new test cases in a separate patch, what I meant was:
>> - use a separate patch (applied *first*) that adds the *new tests* (to be
>> run on existing platforms), i.e. test_ftrace_condition
>> - as part of the patch that actually adds powerpc support, add all the small
>> test case snippets that specifically enable the test cases for powerpc
>> This is again so that each set in a series is meaningful in itself (and
>> does not introduce testsuite regressions when applied alone).
> ...
>> Wei-cheng Wang wrote:
>>> if (tpoint->compiled_cond)
>>> err = ((condfn) (uintptr_t) (tpoint->compiled_cond)) (ctx, &value);
>>>
>>> I think probably either we could pass ctx->regs to compiled_cond instead,
>>> or move the declarations of fast_tracepoint_ctx (and others) to tracepoint.h,
>>> so we can use "offsetof (fast_tracepoint_ctx, regs)" instead.
>>> Any suggestion?
>> FWIW, passing the regs buffer directly to the compiled routine seems
>> more straightforward to me ...
>
I'm afraid I missed this patch before and just posted a patch fixing the
very same issue! I'm sorry about that.
gdb-patches: https://sourceware.org/ml/gdb-patches/2015-09/msg00240.html
bugzilla: https://sourceware.org/bugzilla/show_bug.cgi?id=18955
However, I went down another route when fixing it. Instead of modifying
`condition_true_at_tracepoint' to take the raw registers as argument, I
modified `gdb_agent_get_raw_reg' to take the fast tracepoint context. And
since this context contains the regcache already, we can collect registers
in an architecture independent manner.
Any thoughts?
> Some of the new cases are used to testing emit-reg, and emit-reg for x86
> doesn't work due to the incorrect argument to compiled_cond - "regs" buffer
> is expected, but tracepoint context is passed
>
> This case also includes the fix for complied_cond in order for x86 to
> pass testing for emit-reg op.
>
> Testing result on my x86_64 Ubuntu 14.04.2 TLS
>
> before w/ new cases only w/ the fix
> PASS 2625 2759 2765
> FAIL 33 39 33
> XFAIL 16 16 16
> KFAIL 2 2 2
> UNTESTED 1 1 1
> UNSUPPORTED 3 3 3
>
> Thanks,
> Wei-cheng
>
> ---
>
> * Fix generating emit-reg op by passing register buffer to compiled_cond.
> * Add a new function for testing compiled-cond by checking whether
> based on a given CONDEXP, a list of expected values should be collected.
>
> ---
>
> gdb/gdbserver/ChangeLog
>
> 2015-06-27 Wei-cheng Wang <cole945@gmail.com>
>
> * tracepoint.c (eval_result_type): Change prototype.
> (condition_true_at_tracepoint): Fix argument to compiled_cond.
>
> gdb/testsuite/ChangeLog
>
> 2015-06-27 Wei-cheng Wang <cole945@gmail.com>
>
> * gdb.trace/ftrace.exp: (test_ftrace_condition) New function
> for testing bytecode compilation.
> ---
> gdb/gdbserver/tracepoint.c | 7 +++--
> gdb/testsuite/gdb.trace/ftrace.exp | 64 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 69 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 57b329d9..fdec7db 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -697,7 +697,7 @@ enum tracepoint_type
>
> struct tracepoint_hit_ctx;
>
> -typedef enum eval_result_type (*condfn) (struct tracepoint_hit_ctx *,
> +typedef enum eval_result_type (*condfn) (unsigned char *,
> ULONGEST *);
>
> /* The definition of a tracepoint. */
> @@ -4903,7 +4903,10 @@ condition_true_at_tracepoint (struct tracepoint_hit_ctx *ctx,
> used. */
> #ifdef IN_PROCESS_AGENT
> if (tpoint->compiled_cond)
> - err = ((condfn) (uintptr_t) (tpoint->compiled_cond)) (ctx, &value);
> + {
> + struct fast_tracepoint_ctx *fctx = (struct fast_tracepoint_ctx *) ctx;
> + err = ((condfn) (uintptr_t) (tpoint->compiled_cond)) (fctx->regs, &value);
> + }
> else
> #endif
> {
> diff --git a/gdb/testsuite/gdb.trace/ftrace.exp b/gdb/testsuite/gdb.trace/ftrace.exp
> index f2d8002..a8eb515 100644
> --- a/gdb/testsuite/gdb.trace/ftrace.exp
> +++ b/gdb/testsuite/gdb.trace/ftrace.exp
> @@ -178,6 +178,42 @@ proc test_fast_tracepoints {} {
> }
> }
>
> +# Test compiled-condition
> +# CONDEXP is the condition expression to be compiled.
> +# VAR is the variable to be collected for testing.
> +# LIST is a list of expected values of VAR should be collected
> +# based on the CONDEXP.
> +proc test_ftrace_condition { condexp var list } \
> +{ with_test_prefix "ond $condexp" \
> +{
> + global executable
> + global hex
> +
> + clean_restart ${executable}
> + if ![runto_main] {
> + fail "Can't run to main to check for trace support"
> + return -1
> + }
> +
> + gdb_test "break end" ".*" ""
> + gdb_test "tvariable \$tsv = 0"
> + gdb_test "ftrace set_point if $condexp" "Fast tracepoint .*"
> + gdb_trace_setactions "set action for tracepoint .*" "" \
> + "collect $var" "^$"
> +
> + gdb_test_no_output "tstart" ""
> + gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" ""
> + gdb_test_no_output "tstop" ""
> +
> + set i 0
> + foreach expval $list {
> + gdb_test "tfind" "Found trace frame $i, tracepoint .*" "tfind frame $i"
> + gdb_test "print $var" "\\$\[0-9\]+ = $expval\[\r\n\]" "expect $expval"
> + set i [expr $i + 1]
> + }
> + gdb_test "tfind" "Target failed to find requested trace frame\."
> +}}
> +
> gdb_reinitialize_dir $srcdir/$subdir
>
> if { [gdb_test "info sharedlibrary" ".*${libipa}.*" "IPA loaded"] != 0 } {
> @@ -186,3 +222,31 @@ if { [gdb_test "info sharedlibrary" ".*${libipa}.*" "IPA loaded"] != 0 } {
> }
>
> test_fast_tracepoints
> +
> +# Test conditional goto and simple expression.
> +test_ftrace_condition "globvar > 7" "globvar" { 8 9 10 }
> +test_ftrace_condition "globvar < 4" "globvar" { 1 2 3 }
> +test_ftrace_condition "globvar >= 7" "globvar" { 7 8 9 10 }
> +test_ftrace_condition "globvar <= 4" "globvar" { 1 2 3 4 }
> +test_ftrace_condition "globvar == 5" "globvar" { 5 }
> +test_ftrace_condition "globvar != 5" "globvar" { 1 2 3 4 6 7 8 9 10 }
> +test_ftrace_condition "globvar > 3 && globvar < 7" "globvar" { 4 5 6 }
> +test_ftrace_condition "globvar < 3 || globvar > 7" "globvar" { 1 2 8 9 10 }
> +test_ftrace_condition "(globvar << 2) + 1 == 29" "globvar" { 7 }
> +test_ftrace_condition "(globvar >> 2) == 2" "globvar" { 8 9 10 }
> +
> +# Test emit_call by accessing trace state variables.
> +test_ftrace_condition "(\$tsv = \$tsv + 2) > 10" "globvar" { 6 7 8 9 10 }
I also posted a very similar test case here:
https://sourceware.org/ml/gdb-patches/2015-09/msg00241.html. It might be
good to merge them, what do you think? I realize this test case is more
precise by checking the trace frame numbers explicitly with `tfind'. The
test case I posted only just checks the number of trace frames is as
expected.
However, I'd be more in favour of moving these tests to their own files, and
checking conditions with both the `trace' and `ftrace' commands.
Sorry again for the duplication of work.
Thanks,
Pierre
> +
> +# This expression is used for testing emit_reg.
> +if [is_amd64_regs_target] {
> + set arg0exp "\$rdi"
> +} elseif [is_x86_like_target] {
> + set arg0exp "*(int *) (\$ebp + 8)"
> +} else {
> + set arg0exp ""
> +}
> +
> +if { "$arg0exp" != "" } {
> + test_ftrace_condition "($arg0exp > 500)" "globvar" { 6 7 8 9 10 }
> +}
>
@@ -697,7 +697,7 @@ enum tracepoint_type
struct tracepoint_hit_ctx;
-typedef enum eval_result_type (*condfn) (struct tracepoint_hit_ctx *,
+typedef enum eval_result_type (*condfn) (unsigned char *,
ULONGEST *);
/* The definition of a tracepoint. */
@@ -4903,7 +4903,10 @@ condition_true_at_tracepoint (struct tracepoint_hit_ctx *ctx,
used. */
#ifdef IN_PROCESS_AGENT
if (tpoint->compiled_cond)
- err = ((condfn) (uintptr_t) (tpoint->compiled_cond)) (ctx, &value);
+ {
+ struct fast_tracepoint_ctx *fctx = (struct fast_tracepoint_ctx *) ctx;
+ err = ((condfn) (uintptr_t) (tpoint->compiled_cond)) (fctx->regs, &value);
+ }
else
#endif
{
@@ -178,6 +178,42 @@ proc test_fast_tracepoints {} {
}
}
+# Test compiled-condition
+# CONDEXP is the condition expression to be compiled.
+# VAR is the variable to be collected for testing.
+# LIST is a list of expected values of VAR should be collected
+# based on the CONDEXP.
+proc test_ftrace_condition { condexp var list } \
+{ with_test_prefix "ond $condexp" \
+{
+ global executable
+ global hex
+
+ clean_restart ${executable}
+ if ![runto_main] {
+ fail "Can't run to main to check for trace support"
+ return -1
+ }
+
+ gdb_test "break end" ".*" ""
+ gdb_test "tvariable \$tsv = 0"
+ gdb_test "ftrace set_point if $condexp" "Fast tracepoint .*"
+ gdb_trace_setactions "set action for tracepoint .*" "" \
+ "collect $var" "^$"
+
+ gdb_test_no_output "tstart" ""
+ gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*" ""
+ gdb_test_no_output "tstop" ""
+
+ set i 0
+ foreach expval $list {
+ gdb_test "tfind" "Found trace frame $i, tracepoint .*" "tfind frame $i"
+ gdb_test "print $var" "\\$\[0-9\]+ = $expval\[\r\n\]" "expect $expval"
+ set i [expr $i + 1]
+ }
+ gdb_test "tfind" "Target failed to find requested trace frame\."
+}}
+
gdb_reinitialize_dir $srcdir/$subdir
if { [gdb_test "info sharedlibrary" ".*${libipa}.*" "IPA loaded"] != 0 } {
@@ -186,3 +222,31 @@ if { [gdb_test "info sharedlibrary" ".*${libipa}.*" "IPA loaded"] != 0 } {
}
test_fast_tracepoints
+
+# Test conditional goto and simple expression.
+test_ftrace_condition "globvar > 7" "globvar" { 8 9 10 }
+test_ftrace_condition "globvar < 4" "globvar" { 1 2 3 }
+test_ftrace_condition "globvar >= 7" "globvar" { 7 8 9 10 }
+test_ftrace_condition "globvar <= 4" "globvar" { 1 2 3 4 }
+test_ftrace_condition "globvar == 5" "globvar" { 5 }
+test_ftrace_condition "globvar != 5" "globvar" { 1 2 3 4 6 7 8 9 10 }
+test_ftrace_condition "globvar > 3 && globvar < 7" "globvar" { 4 5 6 }
+test_ftrace_condition "globvar < 3 || globvar > 7" "globvar" { 1 2 8 9 10 }
+test_ftrace_condition "(globvar << 2) + 1 == 29" "globvar" { 7 }
+test_ftrace_condition "(globvar >> 2) == 2" "globvar" { 8 9 10 }
+
+# Test emit_call by accessing trace state variables.
+test_ftrace_condition "(\$tsv = \$tsv + 2) > 10" "globvar" { 6 7 8 9 10 }
+
+# This expression is used for testing emit_reg.
+if [is_amd64_regs_target] {
+ set arg0exp "\$rdi"
+} elseif [is_x86_like_target] {
+ set arg0exp "*(int *) (\$ebp + 8)"
+} else {
+ set arg0exp ""
+}
+
+if { "$arg0exp" != "" } {
+ test_ftrace_condition "($arg0exp > 500)" "globvar" { 6 7 8 9 10 }
+}