From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99147 invoked by alias); 14 Sep 2015 14:04:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 99118 invoked by uid 89); 14 Sep 2015 14:04:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (146.101.78.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 14 Sep 2015 14:04:26 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-37-36PgjkpUTFWpmlbGy3_PzA-1; Mon, 14 Sep 2015 15:04:21 +0100 Received: from [10.2.206.88] ([10.1.2.79]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 14 Sep 2015 15:04:20 +0100 Message-ID: <55F6D3E4.1030109@arm.com> Date: Mon, 14 Sep 2015 14:04:00 -0000 From: Pierre Langlois User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Wei-cheng Wang CC: "uweigand@de.ibm.com" , "gdb-patches@sourceware.org" , pierre.langlois@arm.com, Pedro Alves Subject: Re: [PATCH 3/5 v4] Fix argument to compiled_cond, and add cases for compiled-condition. References: <1435422102-39438-1-git-send-email-cole945@gmail.com> <1435422102-39438-3-git-send-email-cole945@gmail.com> In-Reply-To: <1435422102-39438-3-git-send-email-cole945@gmail.com> X-MC-Unique: 36PgjkpUTFWpmlbGy3_PzA-1 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00290.txt.bz2 Hi Wei-cheng, On 27/06/15 17:21, Wei-cheng Wang wrote: > Hi, >=20 > 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 s= mall >> 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 =3D ((condfn) (uintptr_t) (tpoint->compiled_cond)) (ctx, &value); >>> >>> I think probably either we could pass ctx->regs to compiled_cond instea= d, >>> or move the declarations of fast_tracepoint_ctx (and others) to tracepo= int.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 ... >=20 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=3D18955 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" buff= er > is expected, but tracepoint context is passed >=20 > This case also includes the fix for complied_cond in order for x86 to > pass testing for emit-reg op. >=20 > Testing result on my x86_64 Ubuntu 14.04.2 TLS >=20 > 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 >=20 > Thanks, > Wei-cheng >=20 > --- >=20 > * 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. >=20 > --- >=20 > gdb/gdbserver/ChangeLog >=20 > 2015-06-27 Wei-cheng Wang >=20 > * tracepoint.c (eval_result_type): Change prototype. > (condition_true_at_tracepoint): Fix argument to compiled_cond. >=20 > gdb/testsuite/ChangeLog >=20 > 2015-06-27 Wei-cheng Wang >=20 > * 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(-) >=20 > 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 >=20=20 > struct tracepoint_hit_ctx; >=20=20 > -typedef enum eval_result_type (*condfn) (struct tracepoint_hit_ctx *, > +typedef enum eval_result_type (*condfn) (unsigned char *, > ULONGEST *); >=20=20 > /* The definition of a tracepoint. */ > @@ -4903,7 +4903,10 @@ condition_true_at_tracepoint (struct tracepoint_hi= t_ctx *ctx, > used. */ > #ifdef IN_PROCESS_AGENT > if (tpoint->compiled_cond) > - err =3D ((condfn) (uintptr_t) (tpoint->compiled_cond)) (ctx, &value); > + { > + struct fast_tracepoint_ctx *fctx =3D (struct fast_tracepoint_ctx *= ) ctx; > + err =3D ((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 {} { > } > } >=20=20 > +# 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 =3D 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\]+ =3D $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 >=20=20 > if { [gdb_test "info sharedlibrary" ".*${libipa}.*" "IPA loaded"] !=3D 0= } { > @@ -186,3 +222,31 @@ if { [gdb_test "info sharedlibrary" ".*${libipa}.*" = "IPA loaded"] !=3D 0 } { > } >=20=20 > 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 >=3D 7" "globvar" { 7 8 9 10 } > +test_ftrace_condition "globvar <=3D 4" "globvar" { 1 2 3 4 } > +test_ftrace_condition "globvar =3D=3D 5" "globvar" { 5 } > +test_ftrace_condition "globvar !=3D 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 1= 0 } > +test_ftrace_condition "(globvar << 2) + 1 =3D=3D 29" "globvar" { 7 } > +test_ftrace_condition "(globvar >> 2) =3D=3D 2" "globvar" { 8 9 10 } > + > +# Test emit_call by accessing trace state variables. > +test_ftrace_condition "(\$tsv =3D \$tsv + 2) > 10" "globvar" { 6 7 8 9 1= 0 } 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" !=3D "" } { > + test_ftrace_condition "($arg0exp > 500)" "globvar" { 6 7 8 9 10 } > +} >=20