Re: [patch v8 24/24] record-btrace: add (reverse-)stepping support

Pedro Alves
2013-12-13T19:22:26Z

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 <eliz@gnu.org>
>
> 2013-12-12 Markus Metzger <markus.t.metzger@intel.com>
>
> * btrace.h (btrace_thread_flag): New.
> (struct btrace_thread_info) <flags>: 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. <markus.t.metzger@intel.com>
> +#
> +# 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 <http://www.gnu.org/licenses/>.
> +
> +# 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. <markus.t.metzger@intel.com>
> +#
> +# 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 <http://www.gnu.org/licenses/>.
> +
> +# 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