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

[Pedro Alves <palves at redhat dot com>]

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