From patchwork Mon Sep 14 15:34:28 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 8687 Received: (qmail 71623 invoked by alias); 14 Sep 2015 15:34:45 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 71612 invoked by uid 89); 14 Sep 2015 15:34:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f53.google.com Received: from mail-pa0-f53.google.com (HELO mail-pa0-f53.google.com) (209.85.220.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Mon, 14 Sep 2015 15:34:42 +0000 Received: by padhy16 with SMTP id hy16so147106004pad.1 for ; Mon, 14 Sep 2015 08:34:40 -0700 (PDT) X-Received: by 10.68.233.134 with SMTP id tw6mr37089307pbc.22.1442244880316; Mon, 14 Sep 2015 08:34:40 -0700 (PDT) Received: from E107787-LIN (power-aix.osuosl.org. [140.211.15.154]) by smtp.gmail.com with ESMTPSA id ts1sm16879327pbc.74.2015.09.14.08.34.35 (version=TLS1_2 cipher=AES128-SHA256 bits=128/128); Mon, 14 Sep 2015 08:34:39 -0700 (PDT) From: Yao Qi To: "Ulrich Weigand" Cc: cole945@gmail.com (Wei-cheng Wang), qiyaoltc@gmail.com (Yao Qi), gdb-patches@sourceware.org Subject: Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols. References: <20150914122007.99FD3220B@oc7340732750.ibm.com> Date: Mon, 14 Sep 2015 16:34:28 +0100 In-Reply-To: <20150914122007.99FD3220B@oc7340732750.ibm.com> (Ulrich Weigand's message of "Mon, 14 Sep 2015 14:20:07 +0200 (CEST)") Message-ID: <86y4g93th7.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes "Ulrich Weigand" writes: > However, if just a single tracepoint is invalid, we simply get an > internal error here, which is the problem Wei-cheng is trying to fix. > [ The same gdbarch_fast_tracepoint_valid_at call is done at the time > the tracepoint was installed originally. However, if at that time > the tracepoint was pending, and its location has now been resolved, > that check was not re-done. ] Thanks for the clarification, Ulrich. > > I see two issues here: > > - I agree with you that there's duplicated checks here. The gdbarch > callback gdbarch_fast_tracepoint_valid_at is apparently supposed to > duplicate the logic done in gdbserver. In any case, any tracepoint > considered "valid" by callback is expected to succeed on the target. > > - The ICE can be triggered by normal user action (if the result of > gdbarch_fast_tracepoint_valid_at changes if locations are recomputed, > see above). > > What I had asked Wei-cheng to implement is to fix these two issues: > properly duplicate the validity check for PowerPC, and re-validate > every time locations are resolved. That is fine by me in general, and I attach a reproducer to trigger GDB internal error on x86. Wei-cheng's patch fixes this internal error, and instead, a normal error is emitted. > > Maybe it would indeed be better to move the validity-checking logic > completely to the target side, i.e. always attempt to download the > tracepoint, and react more intelligently if that fails (e.g. downgrade > to a regular tracepoint?). I'm not sure if that is doable with the > current remote protocol definition. Thoughts? The duplicated checks in both GDB and GDBserver sides aren't that bad to me, however, I am worried about using internal knowledge of GDBserver and IPA to validate fast tracepoint in GDB side. I raised this here https://sourceware.org/ml/gdb-patches/2015-09/msg00041.html I suspect that we won't see GDB internal error on PPC if we don't do checks using GDBserver internal knowledge. Without GDBserver internal knowledge, ppc_fast_tracepoint_valid_at should always return true. diff --git a/gdb/testsuite/gdb.trace/pending.exp b/gdb/testsuite/gdb.trace/pending.exp index 9938c5a..b27887a 100644 --- a/gdb/testsuite/gdb.trace/pending.exp +++ b/gdb/testsuite/gdb.trace/pending.exp @@ -492,6 +492,64 @@ proc pending_tracepoint_with_action_resolved { trace_type } \ gdb_test "tfind" "Target failed to find requested trace frame..*" "tfind test frame" }} +# Verify setting tracepoint on a very short instruction. + +proc pending_tracepoint_on_short_insn { trace_type } \ +{ with_test_prefix "$trace_type on short insn" \ +{ + if { ![istarget "x86_64-*"] && ![istarget "i?86-*"] } { + return + } + + global executable + global srcfile + global lib_sl1 + global gdb_prompt + + # Start with a fresh gdb. + clean_restart $executable + if ![runto_main] { + fail "Can't run to main" + return -1 + } + + gdb_test_multiple "$trace_type set_point3" "set pending tracepoint on set_point2" { + -re ".*Make \(fast |\)tracepoint pending.*y or \\\[n\\\]. $" { + gdb_test "y" "\(Fast t|T\)racepoint.*set_point3.*pending." \ + "set pending tracepoint (without symbols)" + } + } + + gdb_test "info trace" \ + "Num Type\[ \]+Disp Enb Address\[ \]+What.* +\[0-9\]+\[\t \]+\(fast |\)tracepoint\[ \]+keep y.*PENDING.*set_point3.*" \ + "single pending tracepoint on set_point3" + + gdb_test "break marker" "Breakpoint.*at.* file .*$srcfile, line.*" \ + "breakpoint on marker" + + gdb_test_no_output "tstart" "start trace experiment" + + gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*pending.c.*" \ + "continue to marker 1" + + set test "continue to marker 2" + gdb_test_multiple "continue" $test { + -re "Continuing.\r\n(Reading .* from remote target...\r\n)?\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { + pass "continue to marker 2" + } + + } + + gdb_test "tstop" "\[\r\n\]+" "stop trace experiment" + + # tracepoint should be resolved. + gdb_test "info trace" \ + "Num Type\[ \]+Disp Enb Address\[ \]+What.* +\[0-9\]+\[\t \]+\(fast |\)tracepoint\[ \]+keep y.*set_point3.*" \ + "tracepoint is resolved" +}} + pending_tracepoint_resolved "trace" pending_tracepoint_works "trace" @@ -506,6 +564,8 @@ pending_tracepoint_with_action_resolved "trace" pending_tracepoint_installed_during_trace "trace" +pending_tracepoint_on_short_insn "trace" + # Re-compile test case with IPA. set libipa [get_in_proc_agent] gdb_load_shlibs $libipa @@ -524,3 +584,4 @@ pending_tracepoint_disconnect_during_trace "ftrace" pending_tracepoint_disconnect_after_resolved "ftrace" pending_tracepoint_with_action_resolved "ftrace" pending_tracepoint_installed_during_trace "ftrace" +pending_tracepoint_on_short_insn "ftrace" diff --git a/gdb/testsuite/gdb.trace/pendshr2.c b/gdb/testsuite/gdb.trace/pendshr2.c index b8a51a5..7d7a2db 100644 --- a/gdb/testsuite/gdb.trace/pendshr2.c +++ b/gdb/testsuite/gdb.trace/pendshr2.c @@ -37,4 +37,19 @@ pendfunc2 (int x) " call " SYMBOL(foo) "\n" #endif ); + + /* `set_point3' is the label where we'll set multiple tracepoints at, + but the instruction at the label isn't large enough to fit a + fast tracepoint jump. */ + asm (" .global " SYMBOL(set_point3) "\n" + SYMBOL(set_point3) ":\n" +#if defined __i386__ + "push %ecx\n" + "pop %ecx\n" +#elif defined __x86_64__ + "pushq %rax\n" + "popq %rax\n" +#else +#endif + ); }