From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7518 invoked by alias); 13 Dec 2013 19:22:26 -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 7508 invoked by uid 89); 13 Dec 2013 19:22:25 -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,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 13 Dec 2013 19:22:24 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBDJMKxA012261 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 13 Dec 2013 14:22:20 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rBDJMITS010657; Fri, 13 Dec 2013 14:22:18 -0500 Message-ID: <52AB5E6A.1010105@redhat.com> Date: Fri, 13 Dec 2013 19:22:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Markus Metzger CC: jan.kratochvil@redhat.com, gdb-patches@sourceware.org, Eli Zaretskii Subject: Re: [patch v8 24/24] record-btrace: add (reverse-)stepping support References: <1386839747-8860-1-git-send-email-markus.t.metzger@intel.com> <1386839747-8860-25-git-send-email-markus.t.metzger@intel.com> In-Reply-To: <1386839747-8860-25-git-send-email-markus.t.metzger@intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-12/txt/msg00540.txt.bz2 On 12/12/2013 09:15 AM, Markus Metzger wrote: > Non-stop mode is not working. Do not allow record-btrace in non-stop mode. Please provide a better rationale/description for the patch. It does much more than that! :-) > CC: Eli Zaretskii > > 2013-12-12 Markus Metzger > > * btrace.h (btrace_thread_flag): New. > (struct btrace_thread_info) : New. > * record-btrace.c (record_btrace_resume_thread, > record_btrace_find_thread_to_move, btrace_step_no_history, > btrace_step_stopped, record_btrace_start_replaying, > record_btrace_step_thread, > record_btrace_find_resume_thread): New. [ The standard way to split the functions across multiple lines is to close the line with ), and reopen the next with (. E.g, * record-btrace.c (record_btrace_resume_thread) (record_btrace_find_thread_to_move, btrace_step_no_history) ... ] > > This recording method may not be available on all processors. > @end table > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c > index 1e8cfc1..bc23f2d 100644 > --- a/gdb/record-btrace.c > +++ b/gdb/record-btrace.c > @@ -167,6 +167,10 @@ record_btrace_open (char *args, int from_tty) > if (!target_supports_btrace ()) > error (_("Target does not support branch tracing.")); > > + if (non_stop) > + error (_("Record btrace can't debug inferior in non-stop mode " > + "(non-stop).")); > + > gdb_assert (record_btrace_thread_observer == NULL); > > disable_chain = make_cleanup (null_cleanup, NULL); > @@ -1233,14 +1237,147 @@ const struct frame_unwind record_btrace_tailcall_frame_unwind = > record_btrace_frame_dealloc_cache > }; > > +/* Indicate that TP should be resumed according to FLAG. */ > + > +static void > +record_btrace_resume_thread (struct thread_info *tp, > + enum btrace_thread_flag flag) > +{ > + struct btrace_thread_info *btinfo; > + > + DEBUG ("resuming %d (%s): %u", tp->num, target_pid_to_str (tp->ptid), flag); > + > + btinfo = &tp->btrace; > + > + if ((btinfo->flags & BTHR_MOVE) != 0) > + error (_("Thread already moving.")); > + > + /* Fetch the latest branch trace. */ > + btrace_fetch (tp); > + > + btinfo->flags |= flag; > +} > + > +/* Find the thread to resume given a PTID. */ > + > +static struct thread_info * > +record_btrace_find_resume_thread (ptid_t ptid) > +{ > + struct thread_info *tp; > + > + /* When asked to resume everything, we pick the current thread. */ > + if (ptid_equal (minus_one_ptid, ptid) || ptid_is_pid (ptid)) > + ptid = inferior_ptid; > + > + return find_thread_ptid (ptid); > +} > + > +/* Stop replaying a thread. */ s/Stop/Start/ > + > +static struct btrace_insn_iterator * > +record_btrace_start_replaying (struct thread_info *tp) > +{ > + volatile struct gdb_exception except; > + struct btrace_insn_iterator *replay; > + struct btrace_thread_info *btinfo; > + int executing; > + > + btinfo = &tp->btrace; > + replay = NULL; > + > + /* We can't start replaying without trace. */ > + if (btinfo->begin == NULL) > + return NULL; > + > + /* Clear the executing flag to allow changes to the current frame. */ > + executing = is_executing (tp->ptid); > + set_executing (tp->ptid, 0); Why is this necessary? Is this so you can start replaying even when the current thread is really executing? > + > + TRY_CATCH (except, RETURN_MASK_ALL) > + { > + struct frame_info *frame; > + struct frame_id frame_id; > + int upd_step_frame_id, upd_step_stack_frame_id; > + > + /* The current frame without replaying - computed via normal unwind. */ > + frame = get_current_frame (); Then this would seem bogus. > + frame_id = get_frame_id (frame); > + > + /* Check if we need to update any stepping-related frame id's. */ > + upd_step_frame_id = frame_id_eq (frame_id, > + tp->control.step_frame_id); > + upd_step_stack_frame_id = frame_id_eq (frame_id, > + tp->control.step_stack_frame_id); > + > + /* We start replaying at the end of the branch trace. This corresponds > + to the current instruction. */ > + replay = xmalloc (sizeof (*replay)); > + btrace_insn_end (replay, btinfo); > + > + /* We're not replaying, yet. */ > + gdb_assert (btinfo->replay == NULL); > + btinfo->replay = replay; > + > + /* Make sure we're not using any stale registers. */ > + registers_changed_ptid (tp->ptid); > + > + /* The current frame with replaying - computed via btrace unwind. */ > + frame = get_current_frame (); > + frame_id = get_frame_id (frame); > + > + /* Replace stepping related frames where necessary. */ > + if (upd_step_frame_id) > + tp->control.step_frame_id = frame_id; > + if (upd_step_stack_frame_id) > + tp->control.step_stack_frame_id = frame_id; Why would this step_frame_id mucking be necessary? Can the thread be stepping when we get here? How? Some comments are missing here. > + } > + > + /* Restore the previous execution state. */ > + set_executing (tp->ptid, executing); > + > + if (except.reason < 0) > + throw_exception (except); If something throws, things are being left possibly in an inconsistent state, it seems to me. Also, "replay" leaks. > /* The to_resume method of target record-btrace. */ > > static void > record_btrace_resume (struct target_ops *ops, ptid_t ptid, int step, > enum gdb_signal signal) > { > + struct thread_info *tp, *other; > + enum btrace_thread_flag flag; > + > + DEBUG ("resume %s: %s", target_pid_to_str (ptid), step ? "step" : "cont"); > + > + tp = record_btrace_find_resume_thread (ptid); > + > + /* Stop replaying other threads if the thread to resume is not replaying. */ > + if (tp != NULL && !btrace_is_replaying (tp) > + && execution_direction != EXEC_REVERSE) > + ALL_THREADS (other) > + record_btrace_stop_replaying (other); Why's that? > + > /* As long as we're not replaying, just forward the request. */ > - if (!record_btrace_is_replaying ()) > + if (!record_btrace_is_replaying () && execution_direction != EXEC_REVERSE) > { > for (ops = ops->beneath; ops != NULL; ops = ops->beneath) > if (ops->to_resume != NULL) > @@ -1249,7 +1386,209 @@ record_btrace_resume (struct target_ops *ops, ptid_t ptid, int step, > error (_("Cannot find target for stepping.")); > } > > - error (_("You can't do this from here. Do 'record goto end', first.")); > + /* Compute the btrace thread flag for the requested move. */ > + if (step == 0) > + flag = execution_direction == EXEC_REVERSE ? BTHR_RCONT : BTHR_CONT; > + else > + flag = execution_direction == EXEC_REVERSE ? BTHR_RSTEP : BTHR_STEP; > + > + /* Find the thread to move. */ > + if (ptid_equal (minus_one_ptid, ptid) || ptid_is_pid (ptid)) > + { > + ALL_THREADS (tp) > + record_btrace_resume_thread (tp, flag); Seems like this steps all threads, when gdb only wants to step inferior_ptid and continue others? > + } > + else if (tp == NULL) > + error (_("Cannot find thread to resume.")); > + else > + record_btrace_resume_thread (tp, flag); > + > + /* We just indicate the resume intent here. The actual stepping happens in > + record_btrace_wait below. */ > +} > + > +/* Find a thread to move. */ > + > +static struct thread_info * > +record_btrace_find_thread_to_move (ptid_t ptid) > +{ > + struct thread_info *tp; > + > + /* First check the parameter thread. */ > + tp = find_thread_ptid (ptid); > + if (tp != NULL && (tp->btrace.flags & BTHR_MOVE) != 0) > + return tp; > + > + /* Next check the current thread. */ > + tp = find_thread_ptid (inferior_ptid); > + if (tp != NULL && (tp->btrace.flags & BTHR_MOVE) != 0) > + return tp; > + > + /* Otherwise, find one other thread that has been resumed. */ > + ALL_THREADS (tp) > + if ((tp->btrace.flags & BTHR_MOVE) != 0) > + return tp; > + > + return NULL; > +} > + > +/* Return a targetwait status indicating that we ran out of history. */ "target_waitstatus" > + > +static struct target_waitstatus > +btrace_step_no_history (void) > +{ > + struct target_waitstatus status; > + > + status.kind = TARGET_WAITKIND_NO_HISTORY; > + > + return status; > +} > + > +/* Return a targetwait status indicating that we stopped. */ Ditto. "we stopped" would lead me to think this should be GDB_SIGNAL_0. I suggest "status indicating that a step finished". > + > +static struct target_waitstatus > +btrace_step_stopped (void) > +{ > + struct target_waitstatus status; > + > + status.kind = TARGET_WAITKIND_STOPPED; > + status.value.sig = GDB_SIGNAL_TRAP; > + > + return status; > +} > + > +/* Step a single thread. */ > + > +static struct target_waitstatus > +record_btrace_step_thread (struct thread_info *tp) > +{ ... > + for (;;) > + { > + const struct btrace_insn *insn; > + > + /* If we can't step any further, we're done. */ > + steps = btrace_insn_prev (replay, 1); > + if (steps == 0) > + return btrace_step_no_history (); > + > + insn = btrace_insn_get (replay); > + gdb_assert (insn); > + > + DEBUG ("stepping %d (%s): reverse~ ... %s", tp->num, Is "reverse~" a typo? > + target_pid_to_str (tp->ptid), > + core_addr_to_string_nz (insn->pc)); > + > + if (breakpoint_here_p (aspace, insn->pc)) > + return btrace_step_stopped (); How is adjust_pc_after_break handled? > + } > + } > } > > diff --git a/gdb/testsuite/gdb.btrace/delta.exp b/gdb/testsuite/gdb.btrace/delta.exp > index 9ee2629..49d151e 100644 > --- a/gdb/testsuite/gdb.btrace/delta.exp > +++ b/gdb/testsuite/gdb.btrace/delta.exp > @@ -61,3 +61,16 @@ gdb_test "record instruction-history /f 1" " > 1\t 0x\[0-9a-f\]+ <\\+\[0-9\]+>:\tmov *\\\$0x0,%eax\r" "delta, 4.2" > gdb_test "record function-call-history /c 1" " > 1\tmain\r" "delta, 4.3" > + > +# check that we can reverse-stepi that instruction > +gdb_test "reverse-stepi" > +gdb_test "info record" " > +Active record target: record-btrace\r > +Recorded 1 instructions in 1 functions for .*\r > +Replay in progress\. At instruction 1\." "delta, 5.1" I don't think assuming \n like that is a good idea. To spell this out in separate lines, I suggest writing a list, and then merging it with \r\n. See dw2-undefined-ret-addr.exp. You may be able to do with {} instead of list. > diff --git a/gdb/testsuite/gdb.btrace/finish.exp b/gdb/testsuite/gdb.btrace/finish.exp > new file mode 100644 > index 0000000..87ebfe1 > --- /dev/null > +++ b/gdb/testsuite/gdb.btrace/finish.exp > @@ -0,0 +1,70 @@ > +# This testcase is part of GDB, the GNU debugger. > +# > +# Copyright 2013 Free Software Foundation, Inc. > +# > +# Contributed by Intel Corp. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# check for btrace support > +if { [skip_btrace_tests] } { return -1 } > + > +# start inferior > +standard_testfile x86-record_goto.S > +if [prepare_for_testing finish.exp $testfile $srcfile] { > + return -1 > +} > + > +if ![runto_main] { > + return -1 > +} > + > +# trace the call to the test function > +gdb_test_no_output "record btrace" > +gdb_test "next" > + > +# let's go somewhere where we can finish > +gdb_test "record goto 32" ".*fun1\.1.*" "finish, 1.1" > +gdb_test "info record" " > +Active record target: record-btrace\r > +Recorded 40 instructions in 16 functions for .*\r > +Replay in progress\. At instruction 32\." "finish, 1.2" > + > +# let's finish into fun2 > +gdb_test "finish" ".*fun2\.3.*" "finish, 2.1" > +gdb_test "info record" " > +Active record target: record-btrace\r > +Recorded 40 instructions in 16 functions for .*\r > +Replay in progress\. At instruction 35\." "finish, 2.2" > + > +# now let's reverse-finish into fun3 > +gdb_test "reverse-finish" ".*fun3\.3.*" "finish, 3.1" > +gdb_test "info record" " > +Active record target: record-btrace\r > +Recorded 40 instructions in 16 functions for .*\r > +Replay in progress\. At instruction 27\." "finish, 3.2" > + > +# finish again - into fun4 > +gdb_test "finish" ".*fun4\.5.*" "finish, 4.1" > +gdb_test "info record" " > +Active record target: record-btrace\r > +Recorded 40 instructions in 16 functions for .*\r > +Replay in progress\. At instruction 39\." "finish, 4.2" > + > +# and reverse-finish again - into main > +gdb_test "reverse-finish" ".*main\.2.*" "finish, 5.1" > +gdb_test "info record" " > +Active record target: record-btrace\r > +Recorded 40 instructions in 16 functions for .*\r > +Replay in progress\. At instruction 1\." "finish, 5.2" Man, this looks way cool. Kudos. > diff --git a/gdb/testsuite/gdb.btrace/multi-thread-step.exp b/gdb/testsuite/gdb.btrace/multi-thread-step.exp > new file mode 100644 > index 0000000..d247845 > --- /dev/null > +++ b/gdb/testsuite/gdb.btrace/multi-thread-step.exp > @@ -0,0 +1,99 @@ > +# This testcase is part of GDB, the GNU debugger. > +# > +# Copyright 2013 Free Software Foundation, Inc. > +# > +# Contributed by Intel Corp. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# check for btrace support > +if { [skip_btrace_tests] } { return -1 } > + > +# start inferior > +standard_testfile > +if {[gdb_compile_pthreads "$srcdir/$subdir/$srcfile" "$binfile" executable {debug}] != "" } { > + return -1 > +} > +clean_restart $testfile > + > +if ![runto_main] { > + return -1 > +} > + > +# set up breakpoints > +set bp_1 [gdb_get_line_number "bp.1" $srcfile] > +set bp_2 [gdb_get_line_number "bp.2" $srcfile] > +set bp_3 [gdb_get_line_number "bp.3" $srcfile] > + > +proc gdb_cont_to_line { line test } { > + gdb_breakpoint $line > + gdb_continue_to_breakpoint "$test" ".*$line\r\n.*" > + delete_breakpoints > +} > + > +# trace the code between the two breakpoints > +delete_breakpoints > +gdb_cont_to_line $srcfile:$bp_1 "mts, 0.1" > +# make sure GDB knows about the new thread > +gdb_test "info threads" ".*" "mts, 0.2" > +gdb_test_no_output "record btrace" "mts, 0.3" > +gdb_cont_to_line $srcfile:$bp_2 "mts, 0.4" > + > +# navigate in the trace history for both threads > +gdb_test "thread 1" ".*" "mts, 1.1" > +gdb_test "record goto begin" ".*" "mts, 1.2" > +gdb_test "info record" ".*Replay in progress\. At instruction 1\." "mts, 1.3" > +gdb_test "thread 2" ".*" "mts, 1.4" > +gdb_test "record goto begin" ".*" "mts, 1.5" > +gdb_test "info record" ".*Replay in progress\. At instruction 1\." "mts, 1.6" What does this "mts" that appears everywhere mean? (with_test_prefix could help here to create more meaningful test names.) > diff --git a/gdb/testsuite/gdb.btrace/rn-dl-bind.exp b/gdb/testsuite/gdb.btrace/rn-dl-bind.exp > new file mode 100644 > index 0000000..4d803f9 > --- /dev/null > +++ b/gdb/testsuite/gdb.btrace/rn-dl-bind.exp What's this testing? I can't infer it from the test name. Please add a comment. -- Pedro Alves