Re: [PATCH 3/5 v4] Fix argument to compiled_cond, and add cases for compiled-condition.

[Pierre Langlois <pierre dot langlois at arm dot com>]

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 }
> +}
>