[PATCH 0/4] gdb, btrace: infrun fixes

[PATCH 0/4] gdb, btrace: infrun fixes

[Markus Metzger]

This small patch series fixes a few issues around reverse/replay stepping that
result in hangs.

Tested with record btrace on IA.

Markus Metzger (4):
  gdb, infrun, btrace: fix reverse/replay stepping at end of execution
    history
  gdb, infrun, record: fix hang when step-over fails with no-history
  gdb, infrun, record: move no-history notification into normal_stop
  gdb, infrun: fix multi-threaded reverse stepping

 gdb/gdbthread.h                               |   7 ++
 gdb/infrun.c                                  |  87 +++++++++++----
 gdb/record-btrace.c                           |  19 ++--
 gdb/testsuite/gdb.btrace/cont-hang.exp        |  47 ++++++++
 .../gdb.btrace/implicit-stop-replaying.exp    | 105 ++++++++++++++++++
 .../gdb.btrace/multi-thread-break-hang.exp    |  92 +++++++++++++++
 gdb/testsuite/gdb.btrace/step-hang.exp        |  46 ++++++++
 gdb/testsuite/gdb.btrace/stepn.exp            |  56 ++++++++++
 8 files changed, 430 insertions(+), 29 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
 create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
 create mode 100644 gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
 create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
 create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp

-- 
2.29.2

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 1/4] gdb, infrun, btrace: fix reverse/replay stepping at end of execution history

[Markus Metzger]

When trying to step over a breakopint at the end of the trace, the
step-over will fail with no-history.  This does not clear step_over_info
so a subsequent resume will cause GDB to not resume the thread and expect
a SIGTRAP to complete the step-over.  This will never come causing GDB to
hang in the wait-for-event poll.

That step-over failed after actually completing the step.  This is wrong.
Fix it by moving the end of execution history check to before we are
stepping.

This exposes another issue, however.  When completing a step-over at the
end of the execution history, we implicitly stop replaying that thread.  A
continue command would resume after the step-over and, since we're no
longer replaying, would continue recording.

Fix that by recording the replay state in the thread's control state and
failing with no-history in keep_going if we're switching from replay to
recording.

gdb/ChangeLog:
2021-01-14  Markus Metzger  <markus.t.metzger@intel.com>

	* gdbthread.h (struct thread_control_state) <is_replaying>: New.
	* infrun.c (clear_proceed_status_thread): Set
	thread_control_state.is_replaying.
	(keep_going_pass_signal): Check thread_control_state.is_replaying.
	* record-btrace.c (record_btrace_single_step_forward): Move end of
	execution history check.

gdb/testsuite/ChangeLog:
2021-01-13  Markus Metzger  <markus.t.metzger@intel.com>

	* gdb.btrace/cont-hang.exp: New file.
	* gdb.btrace/step-hang.exp: New file.
	* gdb.btrace/stepn.exp: New file.
---
 gdb/gdbthread.h                        |  3 ++
 gdb/infrun.c                           | 22 ++++++++++
 gdb/record-btrace.c                    | 19 ++++-----
 gdb/testsuite/gdb.btrace/cont-hang.exp | 47 +++++++++++++++++++++
 gdb/testsuite/gdb.btrace/step-hang.exp | 46 +++++++++++++++++++++
 gdb/testsuite/gdb.btrace/stepn.exp     | 56 ++++++++++++++++++++++++++
 6 files changed, 184 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
 create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
 create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index eef37f79e6a..563b7bd8954 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -159,6 +159,9 @@ struct thread_control_state
      command.  This is used to decide whether "set scheduler-locking
      step" behaves like "on" or "off".  */
   int stepping_command = 0;
+
+  /* Whether the thread was replaying when the command was issued.  */
+  bool is_replaying = false;
 };
 
 /* Inferior thread specific part of `struct infcall_suspend_state'.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index a271220b261..591fda93d21 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2658,6 +2658,8 @@ clear_proceed_status_thread (struct thread_info *tp)
 
   /* Discard any remaining commands or status from previous stop.  */
   bpstat_clear (&tp->control.stop_bpstat);
+
+  tp->control.is_replaying = target_record_is_replaying (tp->ptid);
 }
 
 void
@@ -7813,6 +7815,26 @@ keep_going_pass_signal (struct execution_control_state *ecs)
   gdb_assert (ecs->event_thread->ptid == inferior_ptid);
   gdb_assert (!ecs->event_thread->resumed);
 
+  /* When a thread reaches the end of its execution history, it automatically
+     stops replaying.  This is so the user doesn't need to explicitly stop it
+     with a separate command.
+
+     We do not want a single command (e.g. continue) to transition from
+     replaying to recording, though, e.g. when starting from a breakpoint we
+     needed to step over at the end of the trace.  When we reach the end of the
+     execution history during stepping, stop with no-history.
+
+     The other direction is fine.  When we're at the end of the execution
+     history, we may reverse-continue to start replaying.  */
+  if (ecs->event_thread->control.is_replaying
+      && !target_record_is_replaying (ecs->event_thread->ptid))
+    {
+      gdb::observers::no_history.notify ();
+      stop_waiting (ecs);
+      normal_stop ();
+      return;
+    }
+
   /* Save the pc before execution, to compare with pc after stop.  */
   ecs->event_thread->prev_pc
     = regcache_read_pc_protected (get_thread_regcache (ecs->event_thread));
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index d9cc7a3b6d8..c36f3350b29 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2350,6 +2350,16 @@ record_btrace_single_step_forward (struct thread_info *tp)
   if (replay == NULL)
     return btrace_step_no_history ();
 
+  /* The execution trace contains (and ends with) the current instruction.
+     This instruction has not been executed, yet, so the trace really ends
+     one instruction earlier.
+
+     We'd fail later on in btrace_insn_next () but we must not trigger
+     breakpoints as we're not really able to step.  */
+  btrace_insn_end (&end, btinfo);
+  if (btrace_insn_cmp (replay, &end) == 0)
+    return btrace_step_no_history ();
+
   /* Check if we're stepping a breakpoint.  */
   if (record_btrace_replay_at_breakpoint (tp))
     return btrace_step_stopped ();
@@ -2372,15 +2382,6 @@ record_btrace_single_step_forward (struct thread_info *tp)
     }
   while (btrace_insn_get (replay) == NULL);
 
-  /* Determine the end of the instruction trace.  */
-  btrace_insn_end (&end, btinfo);
-
-  /* The execution trace contains (and ends with) the current instruction.
-     This instruction has not been executed, yet, so the trace really ends
-     one instruction earlier.  */
-  if (btrace_insn_cmp (replay, &end) == 0)
-    return btrace_step_no_history ();
-
   return btrace_step_spurious ();
 }
 
diff --git a/gdb/testsuite/gdb.btrace/cont-hang.exp b/gdb/testsuite/gdb.btrace/cont-hang.exp
new file mode 100644
index 00000000000..59d0e17599e
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/cont-hang.exp
@@ -0,0 +1,47 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2021 Free Software Foundation, Inc.
+#
+# 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/>.
+
+# Test that we do not hang when trying to continue over a breakpoint at
+# the end of the trace.
+
+if { [skip_btrace_tests] } {
+    unsupported "target does not support record-btrace"
+    return -1
+}
+
+standard_testfile record_goto.c
+if [prepare_for_testing "failed to prepare" $testfile $srcfile] {
+    return -1
+}
+
+if ![runto_main] {
+    untested "failed to run to main"
+    return -1
+}
+
+# Trace the call to the test function.
+gdb_test_no_output "record btrace"
+gdb_test "next" "main\.3.*"
+
+# We need to be replaying, otherwise, we'd just continue recording.
+gdb_test "reverse-stepi"
+gdb_test "break"
+
+# Continuing will step over the breakpoint and then run into the end of
+# the execution history.  This ends replay, so we can continue recording.
+gdb_test "continue" "No more reverse-execution history.*"
+gdb_continue_to_end
diff --git a/gdb/testsuite/gdb.btrace/step-hang.exp b/gdb/testsuite/gdb.btrace/step-hang.exp
new file mode 100644
index 00000000000..e175ff5c35d
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/step-hang.exp
@@ -0,0 +1,46 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2021 Free Software Foundation, Inc.
+#
+# 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/>.
+
+# Test that we do not hang when trying to step over a breakpoint at the
+# end of the trace.
+
+if { [skip_btrace_tests] } {
+    unsupported "target does not support record-btrace"
+    return -1
+}
+
+standard_testfile record_goto.c
+if [prepare_for_testing "failed to prepare" $testfile $srcfile] {
+    return -1
+}
+
+if ![runto_main] {
+    untested "failed to run to main"
+    return -1
+}
+
+# Trace the call to the test function.
+gdb_test_no_output "record btrace"
+gdb_test "next" "main\.3.*"
+
+# We need to be replaying, otherwise, we'd just continue recording.
+gdb_test "reverse-stepi"
+gdb_test "break"
+
+# Stepping over the breakpoint ends replaying and we can continue recording.
+gdb_test "step"  "main\.3.*"
+gdb_continue_to_end
diff --git a/gdb/testsuite/gdb.btrace/stepn.exp b/gdb/testsuite/gdb.btrace/stepn.exp
new file mode 100644
index 00000000000..686ca8ceb5e
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/stepn.exp
@@ -0,0 +1,56 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2021 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/>.
+
+# Test that step n does not start recording when issued while replaying.
+
+if { [skip_btrace_tests] } {
+    unsupported "target does not support record-btrace"
+    return -1
+}
+
+standard_testfile record_goto.c
+if [prepare_for_testing "failed to prepare" $testfile $srcfile] {
+    return -1
+}
+
+if ![runto_main] {
+    untested "failed to run to main"
+    return -1
+}
+
+# Trace the call to the test function.
+gdb_test_no_output "record btrace"
+gdb_test "next" "main\.3.*"
+
+# Stepping should bring us to the end of the execution history, but should
+# not resume recording.
+with_test_prefix "stepi" {
+    gdb_test "reverse-stepi"
+    gdb_test "stepi 5" "No more reverse-execution history.*main\.3.*"
+}
+
+with_test_prefix "step" {
+    gdb_test "reverse-step"
+    gdb_test "step 5" "No more reverse-execution history.*main\.3.*"
+}
+
+with_test_prefix "next" {
+    gdb_test "reverse-step"
+    gdb_test "next 5" "No more reverse-execution history.*main\.3.*"
+}
-- 
2.29.2

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 2/4] gdb, infrun, record: fix hang when step-over fails with no-history

[Markus Metzger]

When trying to step over a breakpoint at the end of the trace while
another thread is replaying, the step-over will fail with no-history.
This does not clear step_over_info so a subsequent resume will cause GDB
to not resume the thread and expect a SIGTRAP to complete the step-over.
This will never come causing GDB to hang in the wait-for-event poll.

This is a variant of the issue fixed in the parent commit.  That commit
addressed the issue for a single-threaded process and fixed an issue with
reverse/replay stepping in general.

This commit addresses the issue for a multi-threaded process.  In this
case, the single-step does not complete.

Finish an in-flight step-over when a thread stopped with NO_HISTORY.
Since we did not move, we will simply start the step-over again.

gdb/ChangeLog:
2021-03-02  Markus Metzger  <markus.t.metzger@intel.com>

	* infrun.c (finish_step_over): New declaration.
	(handle_inferior_event): Call finish_step_over.

gdb/testsuite/ChangeLog:
2021-03-02  Markus Metzger  <markus.t.metzger@intel.com>

	* gdb.btrace/multi-thread-break-hang.exp: New file.
---
 gdb/infrun.c                                  |  6 ++
 .../gdb.btrace/multi-thread-break-hang.exp    | 92 +++++++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100644 gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 591fda93d21..e79094ad2b2 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -95,6 +95,8 @@ static void resume (gdb_signal sig);
 
 static void wait_for_inferior (inferior *inf);
 
+static int finish_step_over (struct execution_control_state *ecs);
+
 /* Asynchronous signal handler registered as event loop source for
    when we have pending events ready to be passed to the core.  */
 static struct async_event_handler *infrun_async_inferior_event_token;
@@ -5543,6 +5545,10 @@ handle_inferior_event (struct execution_control_state *ecs)
 	return;
 
       gdb::observers::no_history.notify ();
+
+      /* Cancel an in-flight step-over.  It will not succeed since we
+	 won't be able to step at the end of the execution history.  */
+      finish_step_over (ecs);
       stop_waiting (ecs);
       return;
     }
diff --git a/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp b/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
new file mode 100644
index 00000000000..8496de5b36c
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
@@ -0,0 +1,92 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2021 Free Software Foundation, Inc.
+#
+# 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/>.
+
+# Test that we cancel an in-flight step-over at the end of the execution
+# history as long as some other thread is still replaying.
+#
+# This used to cause GDB to hang in poll ().
+
+if { [skip_btrace_tests] } {
+    unsupported "target does not support record-btrace"
+    return -1
+}
+
+standard_testfile multi-thread-step.c
+if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug libs=-lpthread}] {
+    return -1
+}
+
+if ![runto_main] {
+    untested "failed to run to 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]
+
+# Trace the code between the two breakpoints.
+gdb_breakpoint $srcfile:$bp_1
+gdb_continue_to_breakpoint "continue to bp.1" ".*$srcfile:$bp_1\r\n.*"
+
+# Make sure GDB knows about the new thread.
+gdb_test "info threads" ".*"
+gdb_test_no_output "record btrace"
+
+# We have two threads at or close to bp.1 but handled only one stop event.
+# Remove the breakpoint so we do not need to deal with the 2nd event.
+delete_breakpoints
+gdb_breakpoint $srcfile:$bp_2
+gdb_continue_to_breakpoint "continue to bp.2" ".*$srcfile:$bp_2\r\n.*"
+
+# Determine the thread that reported the breakpoint.
+set thread "bad"
+gdb_test_multiple "thread" "thread" {
+    -re "Current thread is \($decimal\).*\r\n$gdb_prompt $" {
+	set thread $expect_out(1,string)
+    }
+}
+
+# Determine the other thread.
+set other "bad"
+if { $thread == 1 } {
+    set other 2
+} elseif { $thread == 2 } {
+    set other 1
+}
+
+# This test works for scheduler-locking 'on' or 'step'; 'replay' would
+# implicitly stop replaying, avoiding the problem; 'off' would step one
+# and resume the other.  The test would work for the current lock-step
+# implementation.
+gdb_test_no_output "set scheduler-locking step"
+
+# Start replaying the other thread.  This will prevent stepping the thread
+# that reported the event.
+gdb_test "thread apply $other record goto begin" ".*"
+gdb_test "thread apply $other info record" "Replay in progress.*"
+
+# We're at a breakpoint so this triggers step-over.  Since we're at the
+# end of the trace, the step will fail.
+gdb_test "stepi" "No more reverse-execution history.*" "stepi.1"
+
+# We used to hang at the second step since step-over insisted on polling
+# the next event.
+gdb_test "stepi" "No more reverse-execution history.*" "stepi.2"
+
+# Do one more just in case.
+gdb_test "stepi" "No more reverse-execution history.*" "stepi.3"
-- 
2.29.2

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 3/4] gdb, infrun, record: move no-history notification into normal_stop

[Markus Metzger]

Leave calling gdb::observers::no_history.notify to normal_stop based on
the last waitstatus.

gdb/ChangeLog:
2021-01-15  Markus Metzger  <markus.t.metzger@intel.com>

	* infrun.c (handle_inferior_event): Remove no-history notification.
	(keep_going_pass_signal): Likewise.  Set last waitstatus.
	(normal_stop): Notify no-history observers.
---
 gdb/infrun.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index e79094ad2b2..81e31cf0019 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5544,8 +5544,6 @@ handle_inferior_event (struct execution_control_state *ecs)
       if (handle_stop_requested (ecs))
 	return;
 
-      gdb::observers::no_history.notify ();
-
       /* Cancel an in-flight step-over.  It will not succeed since we
 	 won't be able to step at the end of the execution history.  */
       finish_step_over (ecs);
@@ -7835,7 +7833,8 @@ keep_going_pass_signal (struct execution_control_state *ecs)
   if (ecs->event_thread->control.is_replaying
       && !target_record_is_replaying (ecs->event_thread->ptid))
     {
-      gdb::observers::no_history.notify ();
+      ecs->ws.kind = TARGET_WAITKIND_NO_HISTORY;
+      target_last_waitstatus = ecs->ws;
       stop_waiting (ecs);
       normal_stop ();
       return;
@@ -8463,6 +8462,9 @@ normal_stop (void)
 	return 1;
     }
 
+  if (last.kind == TARGET_WAITKIND_NO_HISTORY)
+    gdb::observers::no_history.notify ();
+
   /* Notify observers about the stop.  This is where the interpreters
      print the stop event.  */
   if (inferior_ptid != null_ptid)
-- 
2.29.2

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 4/4] gdb, infrun: fix multi-threaded reverse stepping

[Markus Metzger]

When reverse-stepping a thread that has a pending breakpoint event, the
thread is not resumed as part of the infcmd function.  A first resume
notices the event and returns without resuming the target.

If the corresponding breakpoint has been deleted, event processing results
in a second resume that performs the intended stepping action.  That
resume happens after the infcmd function returned and the temporarily
modified execution direction was restored.

I would have expected everything to be completed by the time the infcmd
function returns but I cannot say whether the behaviour I'm seeing is
intentional.

Assuming it is, this patch addresses the loss of the execution direction
by storing the direction in a thread's control state and changing most of
infrun to take it from there rather than using the global variable.

gdb/ChangeLog:
2021-02-26  Markus Metzger  <markus.t.metzger@intel.com>

	* gdbthread.h: Include "infrun.h".
	(struct thread_control_state) <execution_direction>: New.
	* infrun.c (maybe_software_singlestep): Add thread argument.  Update
	callers.  Use thread's execution direction.
	(clear_proceed_status_thread): Set thread's execution direction.
	(resume_1): Temporarily set the thread's execution direction.
	(keep_going_stepped_thread): Likewise.
	(schedlock_applies): Use thread's execution direction.
	(proceed): Likewise.
	(adjust_pc_after_break): Likewise.
	(process_event_stop_test): Likewise.

gdb/testsuite/ChangeLog:
2021-02-26  Markus Metzger  <markus.t.metzger@intel.com>

	* gdb.btrace/implicit-stop-replaying.exp: New file.
---
 gdb/gdbthread.h                               |   4 +
 gdb/infrun.c                                  |  55 +++++----
 .../gdb.btrace/implicit-stop-replaying.exp    | 105 ++++++++++++++++++
 3 files changed, 145 insertions(+), 19 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 563b7bd8954..32ad0435552 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -33,6 +33,7 @@ struct symtab;
 #include "gdbsupport/common-gdbthread.h"
 #include "gdbsupport/forward-scope-exit.h"
 #include "displaced-stepping.h"
+#include "infrun.h"
 
 struct inferior;
 struct process_stratum_target;
@@ -162,6 +163,9 @@ struct thread_control_state
 
   /* Whether the thread was replaying when the command was issued.  */
   bool is_replaying = false;
+
+  /* The execution direction when the command was issued.  */
+  enum exec_direction_kind execution_direction = EXEC_FORWARD;
 };
 
 /* Inferior thread specific part of `struct infcall_suspend_state'.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 81e31cf0019..efcce5bcc4a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -89,7 +89,8 @@ static void insert_step_resume_breakpoint_at_caller (struct frame_info *);
 
 static void insert_longjmp_resume_breakpoint (struct gdbarch *, CORE_ADDR);
 
-static bool maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc);
+static bool maybe_software_singlestep (const struct thread_info *tp,
+				       struct gdbarch *gdbarch, CORE_ADDR pc);
 
 static void resume (gdb_signal sig);
 
@@ -2048,11 +2049,12 @@ bool sched_multi = false;
    PC the location to step over.  */
 
 static bool
-maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc)
+maybe_software_singlestep (const struct thread_info *tp,
+			   struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   bool hw_step = true;
 
-  if (execution_direction == EXEC_FORWARD
+  if (tp->control.execution_direction == EXEC_FORWARD
       && gdbarch_software_single_step_p (gdbarch))
     hw_step = !insert_single_step_breakpoints (gdbarch);
 
@@ -2336,6 +2338,11 @@ resume_1 (enum gdb_signal sig)
 	      insert_breakpoints ();
 
 	      resume_ptid = internal_resume_ptid (user_step);
+
+	      scoped_restore save_exec_dir
+		= make_scoped_restore (&execution_direction,
+				       tp->control.execution_direction);
+
 	      do_target_resume (resume_ptid, false, GDB_SIGNAL_0);
 	      tp->resumed = true;
 	      return;
@@ -2385,7 +2392,7 @@ resume_1 (enum gdb_signal sig)
 	  set_step_over_info (regcache->aspace (),
 			      regcache_read_pc (regcache), 0, tp->global_num);
 
-	  step = maybe_software_singlestep (gdbarch, pc);
+	  step = maybe_software_singlestep (tp, gdbarch, pc);
 
 	  insert_breakpoints ();
 	}
@@ -2404,7 +2411,7 @@ resume_1 (enum gdb_signal sig)
 
   /* Do we need to do it the hard way, w/temp breakpoints?  */
   else if (step)
-    step = maybe_software_singlestep (gdbarch, pc);
+    step = maybe_software_singlestep (tp, gdbarch, pc);
 
   /* Currently, our software single-step implementation leads to different
      results than hardware single-stepping in one situation: when stepping
@@ -2475,7 +2482,7 @@ resume_1 (enum gdb_signal sig)
   else
     resume_ptid = internal_resume_ptid (user_step);
 
-  if (execution_direction != EXEC_REVERSE
+  if (tp->control.execution_direction != EXEC_REVERSE
       && step && breakpoint_inserted_here_p (aspace, pc))
     {
       /* There are two cases where we currently need to step a
@@ -2545,6 +2552,10 @@ resume_1 (enum gdb_signal sig)
       gdb_assert (pc_in_thread_step_range (pc, tp));
     }
 
+  scoped_restore save_exec_dir
+    = make_scoped_restore (&execution_direction,
+			   tp->control.execution_direction);
+
   do_target_resume (resume_ptid, step, sig);
   tp->resumed = true;
 }
@@ -2662,6 +2673,7 @@ clear_proceed_status_thread (struct thread_info *tp)
   bpstat_clear (&tp->control.stop_bpstat);
 
   tp->control.is_replaying = target_record_is_replaying (tp->ptid);
+  tp->control.execution_direction = ::execution_direction;
 }
 
 void
@@ -2761,7 +2773,7 @@ schedlock_applies (struct thread_info *tp)
 	      && tp->control.stepping_command)
 	  || (scheduler_mode == schedlock_replay
 	      && target_record_will_replay (minus_one_ptid,
-					    execution_direction)));
+					    tp->control.execution_direction)));
 }
 
 /* Calls target_commit_resume on all targets.  */
@@ -2903,7 +2915,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
     {
       if (pc == cur_thr->suspend.stop_pc
 	  && breakpoint_here_p (aspace, pc) == ordinary_breakpoint_here
-	  && execution_direction != EXEC_REVERSE)
+	  && cur_thr->control.execution_direction != EXEC_REVERSE)
 	/* There is a breakpoint at the address we will resume at,
 	   step one instruction before inserting breakpoints so that
 	   we do not stop right away (and report a second hit at this
@@ -4135,7 +4147,7 @@ adjust_pc_after_break (struct thread_info *thread,
      breakpoint at PC - 1.  We'd then report a hit on B1, although
      INSN1 hadn't been de-executed yet.  Doing nothing is the correct
      behaviour.  */
-  if (execution_direction == EXEC_REVERSE)
+  if (thread->control.execution_direction == EXEC_REVERSE)
     return;
 
   /* If the target can tell whether the thread hit a SW breakpoint,
@@ -6432,7 +6444,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 
       delete_step_resume_breakpoint (ecs->event_thread);
       if (ecs->event_thread->control.proceed_to_finish
-	  && execution_direction == EXEC_REVERSE)
+	  && ecs->event_thread->control.execution_direction == EXEC_REVERSE)
 	{
 	  struct thread_info *tp = ecs->event_thread;
 
@@ -6447,7 +6459,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 	}
       fill_in_stop_func (gdbarch, ecs);
       if (ecs->event_thread->suspend.stop_pc == ecs->stop_func_start
-	  && execution_direction == EXEC_REVERSE)
+	  && ecs->event_thread->control.execution_direction == EXEC_REVERSE)
 	{
 	  /* We are stepping over a function call in reverse, and just
 	     hit the step-resume breakpoint at the start address of
@@ -6572,7 +6584,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 
   if (pc_in_thread_step_range (ecs->event_thread->suspend.stop_pc,
 			       ecs->event_thread)
-      && (execution_direction != EXEC_REVERSE
+      && (ecs->event_thread->control.execution_direction != EXEC_REVERSE
 	  || frame_id_eq (get_frame_id (frame),
 			  ecs->event_thread->control.step_frame_id)))
     {
@@ -6592,7 +6604,7 @@ process_event_stop_test (struct execution_control_state *ecs)
       CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc;
       if (stop_pc == ecs->event_thread->control.step_range_start
 	  && stop_pc != ecs->stop_func_start
-	  && execution_direction == EXEC_REVERSE)
+	  && ecs->event_thread->control.execution_direction == EXEC_REVERSE)
 	end_stepping_range (ecs);
       else
 	keep_going (ecs);
@@ -6614,7 +6626,7 @@ process_event_stop_test (struct execution_control_state *ecs)
      backward through the trampoline code, and that's handled further
      down, so there is nothing for us to do here.  */
 
-  if (execution_direction != EXEC_REVERSE
+  if (ecs->event_thread->control.execution_direction != EXEC_REVERSE
       && ecs->event_thread->control.step_over_calls == STEP_OVER_UNDEBUGGABLE
       && in_solib_dynsym_resolve_code (ecs->event_thread->suspend.stop_pc))
     {
@@ -6747,7 +6759,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 
       /* Reverse stepping through solib trampolines.  */
 
-      if (execution_direction == EXEC_REVERSE
+      if (ecs->event_thread->control.execution_direction == EXEC_REVERSE
 	  && ecs->event_thread->control.step_over_calls != STEP_OVER_NONE
 	  && (gdbarch_skip_trampoline_code (gdbarch, frame, stop_pc)
 	      || (ecs->stop_func_start == 0
@@ -6775,7 +6787,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 	     stepped into (backwards), and continue to there.  When we
 	     get there, we'll need to single-step back to the caller.  */
 
-	  if (execution_direction == EXEC_REVERSE)
+	  if (ecs->event_thread->control.execution_direction == EXEC_REVERSE)
 	    {
 	      /* If we're already at the start of the function, we've either
 		 just stepped backward into a single instruction function,
@@ -6838,7 +6850,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 						  tmp_sal)
 	    && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
 	  {
-	    if (execution_direction == EXEC_REVERSE)
+	    if (ecs->event_thread->control.execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
 	    else
 	      handle_step_into_function (gdbarch, ecs);
@@ -6856,7 +6868,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 	  return;
 	}
 
-      if (execution_direction == EXEC_REVERSE)
+      if (ecs->event_thread->control.execution_direction == EXEC_REVERSE)
 	{
 	  /* If we're already at the start of the function, we've either just
 	     stepped backward into a single instruction function without line
@@ -6885,7 +6897,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 
   /* Reverse stepping through solib trampolines.  */
 
-  if (execution_direction == EXEC_REVERSE
+  if (ecs->event_thread->control.execution_direction == EXEC_REVERSE
       && ecs->event_thread->control.step_over_calls != STEP_OVER_NONE)
     {
       CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc;
@@ -7404,6 +7416,11 @@ keep_going_stepped_thread (struct thread_info *tp)
 
       tp->resumed = true;
       resume_ptid = internal_resume_ptid (tp->control.stepping_command);
+
+      scoped_restore save_exec_dir
+	= make_scoped_restore (&execution_direction,
+			       tp->control.execution_direction);
+
       do_target_resume (resume_ptid, false, GDB_SIGNAL_0);
     }
   else
diff --git a/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp b/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
new file mode 100644
index 00000000000..2a396d9497f
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
@@ -0,0 +1,105 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2021 Free Software Foundation, Inc.
+#
+# 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/>.
+
+# Test that we stop replaying other threads when stepping a thread at the
+# end of its execution history.
+#
+# This is similar to the last test in multi-thread-step.exp, except that
+# we reverse-step instead of record goto begin to start replaying and we
+# step instead of continuing.
+#
+# This triggered a bug where GDB confused the execution direction and kept
+# stepping both threads backwards instead of forwards.
+
+if { [skip_btrace_tests] } {
+    unsupported "target does not support record-btrace"
+    return -1
+}
+
+standard_testfile multi-thread-step.c
+if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug libs=-lpthread}] {
+    return -1
+}
+
+if ![runto_main] {
+    untested "failed to run to 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]
+
+# Trace the code between the two breakpoints.
+gdb_breakpoint $srcfile:$bp_1
+gdb_continue_to_breakpoint "continue to bp.1" ".*$srcfile:$bp_1\r\n.*"
+
+# Make sure GDB knows about the new thread.
+gdb_test "info threads" ".*"
+gdb_test_no_output "record btrace"
+
+# We have two threads at or close to bp.1 but handled only one stop event.
+# Remove the breakpoint so we do not need to deal with the 2nd event.
+delete_breakpoints
+gdb_breakpoint $srcfile:$bp_2
+gdb_continue_to_breakpoint "continue to bp.2" ".*$srcfile:$bp_2\r\n.*"
+
+# Determine the thread that reported the breakpoint.
+set thread "bad"
+gdb_test_multiple "thread" "thread" {
+    -re "Current thread is \($decimal\).*\r\n$gdb_prompt $" {
+	set thread $expect_out(1,string)
+    }
+}
+
+# Determine the other thread.
+set other "bad"
+if { $thread == 1 } {
+    set other 2
+} elseif { $thread == 2 } {
+    set other 1
+}
+
+# This test only works for scheduler-locking 'replay'.
+gdb_test_no_output "set scheduler-locking replay"
+
+# Remove breakpoints or we might run into it right away.
+delete_breakpoints
+
+# Start replaying the other thread.
+gdb_test "thread apply $other reverse-stepi" ".*"
+gdb_test "thread apply $other info record" "Replay in progress.*"
+
+# Step the thread that reported the breakpoint, which is not replaying.
+gdb_test "next" "return arg;.*"
+
+proc check_not_replaying { thread } {
+    global gdb_prompt
+
+    gdb_test_multiple "thread apply $thread info record" \
+	"thread $thread not replaying" {
+        -re "Replay in progress" {
+            fail $gdb_test_name
+        }
+        -re "$gdb_prompt $" {
+            pass $gdb_test_name
+        }
+    }
+}
+
+check_not_replaying 1
+check_not_replaying 2
-- 
2.29.2

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 0/4] gdb, btrace: infrun fixes

[Metzger, Markus T]

ping

>-----Original Message-----
>From: Metzger, Markus T <markus.t.metzger@intel.com>
>Sent: Tuesday, March 16, 2021 10:35 AM
>To: gdb-patches@sourceware.org
>Subject: [PATCH 0/4] gdb, btrace: infrun fixes
>
>This small patch series fixes a few issues around reverse/replay stepping that
>result in hangs.
>
>Tested with record btrace on IA.
>
>Markus Metzger (4):
>  gdb, infrun, btrace: fix reverse/replay stepping at end of execution
>    history
>  gdb, infrun, record: fix hang when step-over fails with no-history
>  gdb, infrun, record: move no-history notification into normal_stop
>  gdb, infrun: fix multi-threaded reverse stepping
>
> gdb/gdbthread.h                               |   7 ++
> gdb/infrun.c                                  |  87 +++++++++++----
> gdb/record-btrace.c                           |  19 ++--
> gdb/testsuite/gdb.btrace/cont-hang.exp        |  47 ++++++++
> .../gdb.btrace/implicit-stop-replaying.exp    | 105 ++++++++++++++++++
> .../gdb.btrace/multi-thread-break-hang.exp    |  92 +++++++++++++++
> gdb/testsuite/gdb.btrace/step-hang.exp        |  46 ++++++++
> gdb/testsuite/gdb.btrace/stepn.exp            |  56 ++++++++++
> 8 files changed, 430 insertions(+), 29 deletions(-)
> create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
> create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
> create mode 100644 gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
> create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
> create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp
>
>--
>2.29.2

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 0/4] gdb, btrace: infrun fixes

[Simon Marchi]

On 2021-03-16 5:34 a.m., Markus Metzger via Gdb-patches wrote:
> This small patch series fixes a few issues around reverse/replay stepping that
> result in hangs.
> 
> Tested with record btrace on IA.
> 
> Markus Metzger (4):
>   gdb, infrun, btrace: fix reverse/replay stepping at end of execution
>     history
>   gdb, infrun, record: fix hang when step-over fails with no-history
>   gdb, infrun, record: move no-history notification into normal_stop
>   gdb, infrun: fix multi-threaded reverse stepping
> 
>  gdb/gdbthread.h                               |   7 ++
>  gdb/infrun.c                                  |  87 +++++++++++----
>  gdb/record-btrace.c                           |  19 ++--
>  gdb/testsuite/gdb.btrace/cont-hang.exp        |  47 ++++++++
>  .../gdb.btrace/implicit-stop-replaying.exp    | 105 ++++++++++++++++++
>  .../gdb.btrace/multi-thread-break-hang.exp    |  92 +++++++++++++++
>  gdb/testsuite/gdb.btrace/step-hang.exp        |  46 ++++++++
>  gdb/testsuite/gdb.btrace/stepn.exp            |  56 ++++++++++
>  8 files changed, 430 insertions(+), 29 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
>  create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
>  create mode 100644 gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
>  create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
>  create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp
> 

I went through the patches.  I know next to nothing about the reverse
execution mechanism, and don't really have time these days to get up to
speed to understand the problems and associated fixes, so I hope that
someone more qualified can take a look and approve (although I know that
this area has not been touched by many people, so I don't know who that
would be).

All I can say is that the tests look well written and adequately
commented, such that if I ever had to debug them, I would be able to
understand the original intentions.

Simon

RE: [PATCH 0/4] gdb, btrace: infrun fixes

[Metzger, Markus T]

Thanks, Simon,

Adding Pedro and Tom.  Maybe they can help review the infrun changes or recommend
someone who is familiar with that code.

Regards,
Markus.

>> This small patch series fixes a few issues around reverse/replay stepping that
>> result in hangs.
>>
>> Tested with record btrace on IA.
>>
>> Markus Metzger (4):
>>   gdb, infrun, btrace: fix reverse/replay stepping at end of execution
>>     history
>>   gdb, infrun, record: fix hang when step-over fails with no-history
>>   gdb, infrun, record: move no-history notification into normal_stop
>>   gdb, infrun: fix multi-threaded reverse stepping
>>
>>  gdb/gdbthread.h                               |   7 ++
>>  gdb/infrun.c                                  |  87 +++++++++++----
>>  gdb/record-btrace.c                           |  19 ++--
>>  gdb/testsuite/gdb.btrace/cont-hang.exp        |  47 ++++++++
>>  .../gdb.btrace/implicit-stop-replaying.exp    | 105 ++++++++++++++++++
>>  .../gdb.btrace/multi-thread-break-hang.exp    |  92 +++++++++++++++
>>  gdb/testsuite/gdb.btrace/step-hang.exp        |  46 ++++++++
>>  gdb/testsuite/gdb.btrace/stepn.exp            |  56 ++++++++++
>>  8 files changed, 430 insertions(+), 29 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
>>  create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
>>  create mode 100644 gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
>>  create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
>>  create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp
>>
>
>I went through the patches.  I know next to nothing about the reverse
>execution mechanism, and don't really have time these days to get up to
>speed to understand the problems and associated fixes, so I hope that
>someone more qualified can take a look and approve (although I know that
>this area has not been touched by many people, so I don't know who that
>would be).
>
>All I can say is that the tests look well written and adequately
>commented, such that if I ever had to debug them, I would be able to
>understand the original intentions.
>
>Simon
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 0/4] gdb, btrace: infrun fixes

[Metzger, Markus T]

Ping.

>-----Original Message-----
>From: Metzger, Markus T <markus.t.metzger@intel.com>
>Sent: Tuesday, June 8, 2021 11:06 AM
>To: Simon Marchi <simon.marchi@polymtl.ca>; tom@tromey.com; Pedro Alves
><pedro@palves.net>
>Cc: gdb-patches@sourceware.org
>Subject: RE: [PATCH 0/4] gdb, btrace: infrun fixes
>
>Thanks, Simon,
>
>Adding Pedro and Tom.  Maybe they can help review the infrun changes or
>recommend
>someone who is familiar with that code.
>
>Regards,
>Markus.
>
>>> This small patch series fixes a few issues around reverse/replay stepping that
>>> result in hangs.
>>>
>>> Tested with record btrace on IA.
>>>
>>> Markus Metzger (4):
>>>   gdb, infrun, btrace: fix reverse/replay stepping at end of execution
>>>     history
>>>   gdb, infrun, record: fix hang when step-over fails with no-history
>>>   gdb, infrun, record: move no-history notification into normal_stop
>>>   gdb, infrun: fix multi-threaded reverse stepping
>>>
>>>  gdb/gdbthread.h                               |   7 ++
>>>  gdb/infrun.c                                  |  87 +++++++++++----
>>>  gdb/record-btrace.c                           |  19 ++--
>>>  gdb/testsuite/gdb.btrace/cont-hang.exp        |  47 ++++++++
>>>  .../gdb.btrace/implicit-stop-replaying.exp    | 105 ++++++++++++++++++
>>>  .../gdb.btrace/multi-thread-break-hang.exp    |  92 +++++++++++++++
>>>  gdb/testsuite/gdb.btrace/step-hang.exp        |  46 ++++++++
>>>  gdb/testsuite/gdb.btrace/stepn.exp            |  56 ++++++++++
>>>  8 files changed, 430 insertions(+), 29 deletions(-)
>>>  create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
>>>  create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
>>>  create mode 100644 gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
>>>  create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
>>>  create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp
>>>
>>
>>I went through the patches.  I know next to nothing about the reverse
>>execution mechanism, and don't really have time these days to get up to
>>speed to understand the problems and associated fixes, so I hope that
>>someone more qualified can take a look and approve (although I know that
>>this area has not been touched by many people, so I don't know who that
>>would be).
>>
>>All I can say is that the tests look well written and adequately
>>commented, such that if I ever had to debug them, I would be able to
>>understand the original intentions.
>>
>>Simon
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 0/4] gdb, btrace: infrun fixes

[Metzger, Markus T]

ping

>-----Original Message-----
>From: Metzger, Markus T <markus.t.metzger@intel.com>
>Sent: Wednesday, July 28, 2021 8:42 AM
>To: gdb-patches@sourceware.org
>Cc: Simon Marchi <simon.marchi@polymtl.ca>; tom@tromey.com; Pedro Alves
><pedro@palves.net>
>Subject: RE: [PATCH 0/4] gdb, btrace: infrun fixes
>
>Ping.
>
>>-----Original Message-----
>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>Sent: Tuesday, June 8, 2021 11:06 AM
>>To: Simon Marchi <simon.marchi@polymtl.ca>; tom@tromey.com; Pedro Alves
>><pedro@palves.net>
>>Cc: gdb-patches@sourceware.org
>>Subject: RE: [PATCH 0/4] gdb, btrace: infrun fixes
>>
>>Thanks, Simon,
>>
>>Adding Pedro and Tom.  Maybe they can help review the infrun changes or
>>recommend
>>someone who is familiar with that code.
>>
>>Regards,
>>Markus.
>>
>>>> This small patch series fixes a few issues around reverse/replay stepping that
>>>> result in hangs.
>>>>
>>>> Tested with record btrace on IA.
>>>>
>>>> Markus Metzger (4):
>>>>   gdb, infrun, btrace: fix reverse/replay stepping at end of execution
>>>>     history
>>>>   gdb, infrun, record: fix hang when step-over fails with no-history
>>>>   gdb, infrun, record: move no-history notification into normal_stop
>>>>   gdb, infrun: fix multi-threaded reverse stepping
>>>>
>>>>  gdb/gdbthread.h                               |   7 ++
>>>>  gdb/infrun.c                                  |  87 +++++++++++----
>>>>  gdb/record-btrace.c                           |  19 ++--
>>>>  gdb/testsuite/gdb.btrace/cont-hang.exp        |  47 ++++++++
>>>>  .../gdb.btrace/implicit-stop-replaying.exp    | 105 ++++++++++++++++++
>>>>  .../gdb.btrace/multi-thread-break-hang.exp    |  92 +++++++++++++++
>>>>  gdb/testsuite/gdb.btrace/step-hang.exp        |  46 ++++++++
>>>>  gdb/testsuite/gdb.btrace/stepn.exp            |  56 ++++++++++
>>>>  8 files changed, 430 insertions(+), 29 deletions(-)
>>>>  create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
>>>>  create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
>>>>  create mode 100644 gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
>>>>  create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
>>>>  create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp
>>>>
>>>
>>>I went through the patches.  I know next to nothing about the reverse
>>>execution mechanism, and don't really have time these days to get up to
>>>speed to understand the problems and associated fixes, so I hope that
>>>someone more qualified can take a look and approve (although I know that
>>>this area has not been touched by many people, so I don't know who that
>>>would be).
>>>
>>>All I can say is that the tests look well written and adequately
>>>commented, such that if I ever had to debug them, I would be able to
>>>understand the original intentions.
>>>
>>>Simon
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/4] gdb, infrun, btrace: fix reverse/replay stepping at end of execution history

[Pedro Alves]

Hi Markus,

Sorry for the delay.  FWIW, I've tried to review this a number of times, but I keep
getting stuck on not understanding the description fully.  I wonder if it's possible
to reword it to make it clearer.

On 16/03/21 09:34, Markus Metzger via Gdb-patches wrote:
> When trying to step over a breakopint at the end of the trace, the

typo: "breakopint"

> step-over will fail with no-history.  This does not clear step_over_info
> so a subsequent resume will cause GDB to not resume the thread and expect
> a SIGTRAP to complete the step-over.  This will never come causing GDB to
> hang in the wait-for-event poll.
> 
> That step-over failed after actually completing the step.  This is wrong.

These last two sentences confused me, it is not clear to me what you're
saying is wrong.  I.e., what exactly does "fail" mean here?  And what exactly
is wrong?

Debugging the testcases a bit, I think I see what you mean.  It's:

 That step-over actually completed the step, and so should not have
 returned no-history, but instead a normal single-step stop.  

Correct?

> Fix it by moving the end of execution history check to before we are
> stepping.
> 
> This exposes another issue, however.  When completing a step-over at the
> end of the execution history, we implicitly stop replaying that thread. 

OOC, where does that happen (implicitly stop replaying)?

> A
> continue command would resume after the step-over and, since we're no
> longer replaying, would continue recording.
> 

This approach relies on the reverse debugging / replaying being handled by GDB.

I mean, remote targets that support reverse debugging themselves, via the "bc", "bs"
packets (backwards continue/step), and "replaylog" stop reply (TARGET_WAITKIND_NO_HISTORY)
are out of luck since the remote target has no way to implement target_record_is_replaying.

Thus I'm wondering whether target_record_is_replaying() existing is a design mistake, and
I wonder -- should it be the target that is guaranteed to reply with no-history before
implicitly stopping replay?  IOW, don't stop replay until a no-history event is returned.

> Fix that by recording the replay state in the thread's control state and
> failing with no-history in keep_going if we're switching from replay to
> recording.

This affects target record just as well, right?  Would it be possible for
the testcases to be made to test that target as well?

> 
> gdb/ChangeLog:
> 2021-01-14  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* gdbthread.h (struct thread_control_state) <is_replaying>: New.
> 	* infrun.c (clear_proceed_status_thread): Set
> 	thread_control_state.is_replaying.
> 	(keep_going_pass_signal): Check thread_control_state.is_replaying.
> 	* record-btrace.c (record_btrace_single_step_forward): Move end of
> 	execution history check.
> 
> gdb/testsuite/ChangeLog:
> 2021-01-13  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* gdb.btrace/cont-hang.exp: New file.
> 	* gdb.btrace/step-hang.exp: New file.
> 	* gdb.btrace/stepn.exp: New file.
> ---
>  gdb/gdbthread.h                        |  3 ++
>  gdb/infrun.c                           | 22 ++++++++++
>  gdb/record-btrace.c                    | 19 ++++-----
>  gdb/testsuite/gdb.btrace/cont-hang.exp | 47 +++++++++++++++++++++
>  gdb/testsuite/gdb.btrace/step-hang.exp | 46 +++++++++++++++++++++
>  gdb/testsuite/gdb.btrace/stepn.exp     | 56 ++++++++++++++++++++++++++
>  6 files changed, 184 insertions(+), 9 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp
>  create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp
>  create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp
> 
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index eef37f79e6a..563b7bd8954 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -159,6 +159,9 @@ struct thread_control_state
>       command.  This is used to decide whether "set scheduler-locking
>       step" behaves like "on" or "off".  */
>    int stepping_command = 0;
> +
> +  /* Whether the thread was replaying when the command was issued.  */
> +  bool is_replaying = false;
>  };
>  
>  /* Inferior thread specific part of `struct infcall_suspend_state'.  */
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index a271220b261..591fda93d21 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2658,6 +2658,8 @@ clear_proceed_status_thread (struct thread_info *tp)
>  
>    /* Discard any remaining commands or status from previous stop.  */
>    bpstat_clear (&tp->control.stop_bpstat);
> +
> +  tp->control.is_replaying = target_record_is_replaying (tp->ptid);
>  }
>  
>  void
> @@ -7813,6 +7815,26 @@ keep_going_pass_signal (struct execution_control_state *ecs)
>    gdb_assert (ecs->event_thread->ptid == inferior_ptid);
>    gdb_assert (!ecs->event_thread->resumed);
>  
> +  /* When a thread reaches the end of its execution history, it automatically
> +     stops replaying.  This is so the user doesn't need to explicitly stop it
> +     with a separate command.
> +
> +     We do not want a single command (e.g. continue) to transition from
> +     replaying to recording, though, e.g. when starting from a breakpoint we
> +     needed to step over at the end of the trace.  When we reach the end of the
> +     execution history during stepping, stop with no-history.
> +
> +     The other direction is fine.  When we're at the end of the execution
> +     history, we may reverse-continue to start replaying.  */
> +  if (ecs->event_thread->control.is_replaying
> +      && !target_record_is_replaying (ecs->event_thread->ptid))
> +    {
> +      gdb::observers::no_history.notify ();
> +      stop_waiting (ecs);
> +      normal_stop ();

normal_stop looks at the last target status to decide what to do.  So here we should call
set_last_target_status before calling normal_stop, I'd think.

> +      return;
> +    }
> +

Re: [PATCH 2/4] gdb, infrun, record: fix hang when step-over fails with no-history

[Pedro Alves]

On 2021-03-16 09:34, Markus Metzger via Gdb-patches wrote:
> When trying to step over a breakpoint at the end of the trace while
> another thread is replaying, the step-over will fail with no-history.

Some information seems missing here.  The no-history event is for the other thread that
is replaying, right?  We don't use displaced stepping when using a record target, so why did that
other thread report the no-history event?  Without displaced stepping, GDB will only resume the
thread stepping over the breakpoint, so I'm confused on how the other thread's position
in the replay log affects the stepping thread.

> This does not clear step_over_info so a subsequent resume will cause GDB
> to not resume the thread and expect a SIGTRAP to complete the step-over.
> This will never come causing GDB to hang in the wait-for-event poll.
> 
> This is a variant of the issue fixed in the parent commit.  That commit
> addressed the issue for a single-threaded process and fixed an issue with
> reverse/replay stepping in general.
> 
> This commit addresses the issue for a multi-threaded process.  In this
> case, the single-step does not complete.
> 
> Finish an in-flight step-over when a thread stopped with NO_HISTORY.
> Since we did not move, we will simply start the step-over again.
> 
> gdb/ChangeLog:
> 2021-03-02  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* infrun.c (finish_step_over): New declaration.
> 	(handle_inferior_event): Call finish_step_over.
> 
> gdb/testsuite/ChangeLog:
> 2021-03-02  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* gdb.btrace/multi-thread-break-hang.exp: New file.
> ---
>  gdb/infrun.c                                  |  6 ++
>  .../gdb.btrace/multi-thread-break-hang.exp    | 92 +++++++++++++++++++
>  2 files changed, 98 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 591fda93d21..e79094ad2b2 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -95,6 +95,8 @@ static void resume (gdb_signal sig);
>  
>  static void wait_for_inferior (inferior *inf);
>  
> +static int finish_step_over (struct execution_control_state *ecs);
> +
>  /* Asynchronous signal handler registered as event loop source for
>     when we have pending events ready to be passed to the core.  */
>  static struct async_event_handler *infrun_async_inferior_event_token;
> @@ -5543,6 +5545,10 @@ handle_inferior_event (struct execution_control_state *ecs)
>  	return;
>  
>        gdb::observers::no_history.notify ();
> +
> +      /* Cancel an in-flight step-over.  It will not succeed since we
> +	 won't be able to step at the end of the execution history.  */
> +      finish_step_over (ecs);
>        stop_waiting (ecs);
>        return;
>      }
> diff --git a/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp b/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
> new file mode 100644
> index 00000000000..8496de5b36c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
> @@ -0,0 +1,92 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2021 Free Software Foundation, Inc.
> +#
> +# 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/>.
> +
> +# Test that we cancel an in-flight step-over at the end of the execution
> +# history as long as some other thread is still replaying.
> +#
> +# This used to cause GDB to hang in poll ().
> +
> +if { [skip_btrace_tests] } {
> +    unsupported "target does not support record-btrace"
> +    return -1
> +}
> +
> +standard_testfile multi-thread-step.c
> +if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug libs=-lpthread}] {

Should use the "pthreads" option as in "{debug pthreads}" instead of linking
with -lpthread directly.

> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "failed to run to 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]
> +
> +# Trace the code between the two breakpoints.
> +gdb_breakpoint $srcfile:$bp_1
> +gdb_continue_to_breakpoint "continue to bp.1" ".*$srcfile:$bp_1\r\n.*"
> +
> +# Make sure GDB knows about the new thread.

This is not necessary nowadays, as normal_stop always calls update_thread_list.

> +gdb_test "info threads" ".*"
> +gdb_test_no_output "record btrace"
> +
> +# We have two threads at or close to bp.1 but handled only one stop event.
> +# Remove the breakpoint so we do not need to deal with the 2nd event.
> +delete_breakpoints
> +gdb_breakpoint $srcfile:$bp_2
> +gdb_continue_to_breakpoint "continue to bp.2" ".*$srcfile:$bp_2\r\n.*"
> +
> +# Determine the thread that reported the breakpoint.
> +set thread "bad"
> +gdb_test_multiple "thread" "thread" {
> +    -re "Current thread is \($decimal\).*\r\n$gdb_prompt $" {
> +	set thread $expect_out(1,string)
> +    }
> +}

This is fine, but FYI, it could be done with a single liner:

  set thread [get_integer_valueof "\$_thread" bad]

> +
> +# Determine the other thread.
> +set other "bad"
> +if { $thread == 1 } {
> +    set other 2
> +} elseif { $thread == 2 } {
> +    set other 1
> +}
> +
> +# This test works for scheduler-locking 'on' or 'step'; 'replay' would
> +# implicitly stop replaying, avoiding the problem; 'off' would step one
> +# and resume the other.  The test would work for the current lock-step
> +# implementation.

What does the last sentence about "lock-step" mean?  Also, I find "work" ambiguous.
I take it here is means "exercises the original bug".  How about saying that?

> +gdb_test_no_output "set scheduler-locking step"
> +
> +# Start replaying the other thread.  This will prevent stepping the thread
> +# that reported the event.

What is the "this" in "This will prevent"?  Is it the use of "record goto" you're
referring to, or something else?

> +gdb_test "thread apply $other record goto begin" ".*"
> +gdb_test "thread apply $other info record" "Replay in progress.*"
> +
> +# We're at a breakpoint so this triggers step-over.  Since we're at the
> +# end of the trace, the step will fail.

What is "we" here?  It's the "other" thread, not the stepping thread, right?
Please clarify the comments.

> +gdb_test "stepi" "No more reverse-execution history.*" "stepi.1"
> +
> +# We used to hang at the second step since step-over insisted on polling
> +# the next event.
> +gdb_test "stepi" "No more reverse-execution history.*" "stepi.2"
> +
> +# Do one more just in case.
> +gdb_test "stepi" "No more reverse-execution history.*" "stepi.3"
> 

Re: [PATCH 3/4] gdb, infrun, record: move no-history notification into normal_stop

[Pedro Alves]

Hi Markus,

On 2021-03-16 09:35, Markus Metzger via Gdb-patches wrote:
> Leave calling gdb::observers::no_history.notify to normal_stop based on
> the last waitstatus.
> 
> gdb/ChangeLog:
> 2021-01-15  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* infrun.c (handle_inferior_event): Remove no-history notification.
> 	(keep_going_pass_signal): Likewise.  Set last waitstatus.
> 	(normal_stop): Notify no-history observers.
> ---
>  gdb/infrun.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index e79094ad2b2..81e31cf0019 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -5544,8 +5544,6 @@ handle_inferior_event (struct execution_control_state *ecs)
>        if (handle_stop_requested (ecs))
>  	return;
>  
> -      gdb::observers::no_history.notify ();
> -
>        /* Cancel an in-flight step-over.  It will not succeed since we
>  	 won't be able to step at the end of the execution history.  */
>        finish_step_over (ecs);
> @@ -7835,7 +7833,8 @@ keep_going_pass_signal (struct execution_control_state *ecs)
>    if (ecs->event_thread->control.is_replaying
>        && !target_record_is_replaying (ecs->event_thread->ptid))
>      {
> -      gdb::observers::no_history.notify ();
> +      ecs->ws.kind = TARGET_WAITKIND_NO_HISTORY;
> +      target_last_waitstatus = ecs->ws;

Ah, you have this here.  This bit about setting the last waitstatus should
be in patch #1, and as mentioned in my review there, use set_last_target_status.

Otherwise this LGTM.

Thanks,
Pedro Alves

>        stop_waiting (ecs);
>        normal_stop ();
>        return;
> @@ -8463,6 +8462,9 @@ normal_stop (void)
>  	return 1;
>      }
>  
> +  if (last.kind == TARGET_WAITKIND_NO_HISTORY)
> +    gdb::observers::no_history.notify ();
> +
>    /* Notify observers about the stop.  This is where the interpreters
>       print the stop event.  */
>    if (inferior_ptid != null_ptid)
> 

Re: [PATCH 4/4] gdb, infrun: fix multi-threaded reverse stepping

[Pedro Alves]

On 2021-03-16 09:35, Markus Metzger via Gdb-patches wrote:
> When reverse-stepping a thread that has a pending breakpoint event, the
> thread is not resumed as part of the infcmd function.  A first resume
> notices the event and returns without resuming the target.
> 
> If the corresponding breakpoint has been deleted, event processing results
> in a second resume that performs the intended stepping action.  That
> resume happens after the infcmd function returned and the temporarily
> modified execution direction was restored.
> 
> I would have expected everything to be completed by the time the infcmd
> function returns but I cannot say whether the behaviour I'm seeing is
> intentional.

The going via the event loop to handle the pending event is intentional, simplifies
things by only having one code path.

(Please adjust the commit log given the new information.)

> 
> Assuming it is, this patch addresses the loss of the execution direction
> by storing the direction in a thread's control state and changing most of
> infrun to take it from there rather than using the global variable.

Do you envision that you'll support having some threads moving forward and
other threads moving backwards, all in the same inferior or target?

Like:

 (gdb) thread 1.1
 (gdb) c&               // thread 1.1 is now continuously recording
 (gdb) thread 1.2
 (gdb) reverse-continue // thread 1.2 is running backwards

Which makes me question -- seems odd to have both the direction recorded in
all the threads, and then still have target_execution_direction() ?

Do we want to eliminate one of these?  Either the target direction, or the threads
direction?

If we don't want to support threads of the same target executing in different directions,
then I suspect that we can reimplement this patch by adding a new target_set_execution_direction
target method, that would be called in the case where infrun skips calling target_resume
because of a pending event.

If we do want to support mixed directions, then is the following snipped from
fetch_inferior_event still useful for anything?

    scoped_restore save_exec_dir
      = make_scoped_restore (&execution_direction,
			     target_execution_direction ());

AFAICS, when handling an event we'll now use the thread's execution direction, so flipping
the global like this here is basically dead code.

> 
> gdb/ChangeLog:
> 2021-02-26  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* gdbthread.h: Include "infrun.h".
> 	(struct thread_control_state) <execution_direction>: New.
> 	* infrun.c (maybe_software_singlestep): Add thread argument.  Update
> 	callers.  Use thread's execution direction.
> 	(clear_proceed_status_thread): Set thread's execution direction.
> 	(resume_1): Temporarily set the thread's execution direction.
> 	(keep_going_stepped_thread): Likewise.
> 	(schedlock_applies): Use thread's execution direction.
> 	(proceed): Likewise.
> 	(adjust_pc_after_break): Likewise.
> 	(process_event_stop_test): Likewise.
> 
> gdb/testsuite/ChangeLog:
> 2021-02-26  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* gdb.btrace/implicit-stop-replaying.exp: New file.
> ---
>  gdb/gdbthread.h                               |   4 +
>  gdb/infrun.c                                  |  55 +++++----
>  .../gdb.btrace/implicit-stop-replaying.exp    | 105 ++++++++++++++++++
>  3 files changed, 145 insertions(+), 19 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
> 
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 563b7bd8954..32ad0435552 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -33,6 +33,7 @@ struct symtab;
>  #include "gdbsupport/common-gdbthread.h"
>  #include "gdbsupport/forward-scope-exit.h"
>  #include "displaced-stepping.h"
> +#include "infrun.h"
>  
>  struct inferior;
>  struct process_stratum_target;
> @@ -162,6 +163,9 @@ struct thread_control_state
>  
>    /* Whether the thread was replaying when the command was issued.  */
>    bool is_replaying = false;
> +
> +  /* The execution direction when the command was issued.  */
> +  enum exec_direction_kind execution_direction = EXEC_FORWARD;
>  };
>  
>  /* Inferior thread specific part of `struct infcall_suspend_state'.  */
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 81e31cf0019..efcce5bcc4a 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -89,7 +89,8 @@ static void insert_step_resume_breakpoint_at_caller (struct frame_info *);
>  
>  static void insert_longjmp_resume_breakpoint (struct gdbarch *, CORE_ADDR);
>  
> -static bool maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc);
> +static bool maybe_software_singlestep (const struct thread_info *tp,

You can just drop the "struct", given C++.

> +				       struct gdbarch *gdbarch, CORE_ADDR pc);
>  
>  static void resume (gdb_signal sig);
>  
> @@ -2048,11 +2049,12 @@ bool sched_multi = false;
>     PC the location to step over.  */
>  
>  static bool
> -maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc)
> +maybe_software_singlestep (const struct thread_info *tp,

Ditto.

> +			   struct gdbarch *gdbarch, CORE_ADDR pc)
>  {
>    bool hw_step = true;
>  
> -  if (execution_direction == EXEC_FORWARD
> +  if (tp->control.execution_direction == EXEC_FORWARD
>        && gdbarch_software_single_step_p (gdbarch))
>      hw_step = !insert_single_step_breakpoints (gdbarch);
>  
> @@ -2336,6 +2338,11 @@ resume_1 (enum gdb_signal sig)
>  	      insert_breakpoints ();
>  
>  	      resume_ptid = internal_resume_ptid (user_step);
> +
> +	      scoped_restore save_exec_dir
> +		= make_scoped_restore (&execution_direction,
> +				       tp->control.execution_direction);
> +
>  	      do_target_resume (resume_ptid, false, GDB_SIGNAL_0);
>  	      tp->resumed = true;
>  	      return;
> @@ -2385,7 +2392,7 @@ resume_1 (enum gdb_signal sig)
>  	  set_step_over_info (regcache->aspace (),
>  			      regcache_read_pc (regcache), 0, tp->global_num);
>  
> -	  step = maybe_software_singlestep (gdbarch, pc);
> +	  step = maybe_software_singlestep (tp, gdbarch, pc);
>  
>  	  insert_breakpoints ();
>  	}
> @@ -2404,7 +2411,7 @@ resume_1 (enum gdb_signal sig)
>  
>    /* Do we need to do it the hard way, w/temp breakpoints?  */
>    else if (step)
> -    step = maybe_software_singlestep (gdbarch, pc);
> +    step = maybe_software_singlestep (tp, gdbarch, pc);
>  
>    /* Currently, our software single-step implementation leads to different
>       results than hardware single-stepping in one situation: when stepping
> @@ -2475,7 +2482,7 @@ resume_1 (enum gdb_signal sig)
>    else
>      resume_ptid = internal_resume_ptid (user_step);
>  
> -  if (execution_direction != EXEC_REVERSE
> +  if (tp->control.execution_direction != EXEC_REVERSE
>        && step && breakpoint_inserted_here_p (aspace, pc))
>      {
>        /* There are two cases where we currently need to step a
> @@ -2545,6 +2552,10 @@ resume_1 (enum gdb_signal sig)
>        gdb_assert (pc_in_thread_step_range (pc, tp));
>      }
>  
> +  scoped_restore save_exec_dir
> +    = make_scoped_restore (&execution_direction,
> +			   tp->control.execution_direction);
> +
>    do_target_resume (resume_ptid, step, sig);
>    tp->resumed = true;
>  }
> @@ -2662,6 +2673,7 @@ clear_proceed_status_thread (struct thread_info *tp)
>    bpstat_clear (&tp->control.stop_bpstat);
>  
>    tp->control.is_replaying = target_record_is_replaying (tp->ptid);
> +  tp->control.execution_direction = ::execution_direction;
>  }
>  
>  void
> @@ -2761,7 +2773,7 @@ schedlock_applies (struct thread_info *tp)
>  	      && tp->control.stepping_command)
>  	  || (scheduler_mode == schedlock_replay
>  	      && target_record_will_replay (minus_one_ptid,
> -					    execution_direction)));
> +					    tp->control.execution_direction)));
>  }
>  
>  /* Calls target_commit_resume on all targets.  */
> @@ -2903,7 +2915,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
>      {
>        if (pc == cur_thr->suspend.stop_pc
>  	  && breakpoint_here_p (aspace, pc) == ordinary_breakpoint_here
> -	  && execution_direction != EXEC_REVERSE)
> +	  && cur_thr->control.execution_direction != EXEC_REVERSE)
>  	/* There is a breakpoint at the address we will resume at,
>  	   step one instruction before inserting breakpoints so that
>  	   we do not stop right away (and report a second hit at this
> @@ -4135,7 +4147,7 @@ adjust_pc_after_break (struct thread_info *thread,
>       breakpoint at PC - 1.  We'd then report a hit on B1, although
>       INSN1 hadn't been de-executed yet.  Doing nothing is the correct
>       behaviour.  */
> -  if (execution_direction == EXEC_REVERSE)
> +  if (thread->control.execution_direction == EXEC_REVERSE)
>      return;
>  
>    /* If the target can tell whether the thread hit a SW breakpoint,
> @@ -6432,7 +6444,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  
>        delete_step_resume_breakpoint (ecs->event_thread);
>        if (ecs->event_thread->control.proceed_to_finish
> -	  && execution_direction == EXEC_REVERSE)
> +	  && ecs->event_thread->control.execution_direction == EXEC_REVERSE)
>  	{
>  	  struct thread_info *tp = ecs->event_thread;
>  
> @@ -6447,7 +6459,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  	}
>        fill_in_stop_func (gdbarch, ecs);
>        if (ecs->event_thread->suspend.stop_pc == ecs->stop_func_start
> -	  && execution_direction == EXEC_REVERSE)
> +	  && ecs->event_thread->control.execution_direction == EXEC_REVERSE)
>  	{
>  	  /* We are stepping over a function call in reverse, and just
>  	     hit the step-resume breakpoint at the start address of
> @@ -6572,7 +6584,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  
>    if (pc_in_thread_step_range (ecs->event_thread->suspend.stop_pc,
>  			       ecs->event_thread)
> -      && (execution_direction != EXEC_REVERSE
> +      && (ecs->event_thread->control.execution_direction != EXEC_REVERSE
>  	  || frame_id_eq (get_frame_id (frame),
>  			  ecs->event_thread->control.step_frame_id)))
>      {
> @@ -6592,7 +6604,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>        CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc;
>        if (stop_pc == ecs->event_thread->control.step_range_start
>  	  && stop_pc != ecs->stop_func_start
> -	  && execution_direction == EXEC_REVERSE)
> +	  && ecs->event_thread->control.execution_direction == EXEC_REVERSE)
>  	end_stepping_range (ecs);
>        else
>  	keep_going (ecs);
> @@ -6614,7 +6626,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>       backward through the trampoline code, and that's handled further
>       down, so there is nothing for us to do here.  */
>  
> -  if (execution_direction != EXEC_REVERSE
> +  if (ecs->event_thread->control.execution_direction != EXEC_REVERSE
>        && ecs->event_thread->control.step_over_calls == STEP_OVER_UNDEBUGGABLE
>        && in_solib_dynsym_resolve_code (ecs->event_thread->suspend.stop_pc))
>      {
> @@ -6747,7 +6759,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  
>        /* Reverse stepping through solib trampolines.  */
>  
> -      if (execution_direction == EXEC_REVERSE
> +      if (ecs->event_thread->control.execution_direction == EXEC_REVERSE
>  	  && ecs->event_thread->control.step_over_calls != STEP_OVER_NONE
>  	  && (gdbarch_skip_trampoline_code (gdbarch, frame, stop_pc)
>  	      || (ecs->stop_func_start == 0
> @@ -6775,7 +6787,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  	     stepped into (backwards), and continue to there.  When we
>  	     get there, we'll need to single-step back to the caller.  */
>  
> -	  if (execution_direction == EXEC_REVERSE)
> +	  if (ecs->event_thread->control.execution_direction == EXEC_REVERSE)
>  	    {
>  	      /* If we're already at the start of the function, we've either
>  		 just stepped backward into a single instruction function,
> @@ -6838,7 +6850,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  						  tmp_sal)
>  	    && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
>  	  {
> -	    if (execution_direction == EXEC_REVERSE)
> +	    if (ecs->event_thread->control.execution_direction == EXEC_REVERSE)
>  	      handle_step_into_function_backward (gdbarch, ecs);
>  	    else
>  	      handle_step_into_function (gdbarch, ecs);
> @@ -6856,7 +6868,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  	  return;
>  	}
>  
> -      if (execution_direction == EXEC_REVERSE)
> +      if (ecs->event_thread->control.execution_direction == EXEC_REVERSE)
>  	{
>  	  /* If we're already at the start of the function, we've either just
>  	     stepped backward into a single instruction function without line
> @@ -6885,7 +6897,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  
>    /* Reverse stepping through solib trampolines.  */
>  
> -  if (execution_direction == EXEC_REVERSE
> +  if (ecs->event_thread->control.execution_direction == EXEC_REVERSE
>        && ecs->event_thread->control.step_over_calls != STEP_OVER_NONE)
>      {
>        CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc;
> @@ -7404,6 +7416,11 @@ keep_going_stepped_thread (struct thread_info *tp)
>  
>        tp->resumed = true;
>        resume_ptid = internal_resume_ptid (tp->control.stepping_command);
> +
> +      scoped_restore save_exec_dir
> +	= make_scoped_restore (&execution_direction,
> +			       tp->control.execution_direction);

Shouldn't this be done for all do_target_resume calls?  How about doing it from
within do_target_resume instead?

> +
>        do_target_resume (resume_ptid, false, GDB_SIGNAL_0);
>      }
>    else
> diff --git a/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp b/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
> new file mode 100644
> index 00000000000..2a396d9497f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp
> @@ -0,0 +1,105 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2021 Free Software Foundation, Inc.
> +#
> +# 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/>.
> +
> +# Test that we stop replaying other threads when stepping a thread at the
> +# end of its execution history.
> +#
> +# This is similar to the last test in multi-thread-step.exp, except that
> +# we reverse-step instead of record goto begin to start replaying and we
> +# step instead of continuing.
> +#
> +# This triggered a bug where GDB confused the execution direction and kept
> +# stepping both threads backwards instead of forwards.
> +
> +if { [skip_btrace_tests] } {
> +    unsupported "target does not support record-btrace"
> +    return -1
> +}
> +
> +standard_testfile multi-thread-step.c
> +if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug libs=-lpthread}] {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "failed to run to 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]
> +
> +# Trace the code between the two breakpoints.
> +gdb_breakpoint $srcfile:$bp_1
> +gdb_continue_to_breakpoint "continue to bp.1" ".*$srcfile:$bp_1\r\n.*"
> +
> +# Make sure GDB knows about the new thread.
> +gdb_test "info threads" ".*"
> +gdb_test_no_output "record btrace"
> +
> +# We have two threads at or close to bp.1 but handled only one stop event.
> +# Remove the breakpoint so we do not need to deal with the 2nd event.
> +delete_breakpoints
> +gdb_breakpoint $srcfile:$bp_2
> +gdb_continue_to_breakpoint "continue to bp.2" ".*$srcfile:$bp_2\r\n.*"
> +
> +# Determine the thread that reported the breakpoint.
> +set thread "bad"
> +gdb_test_multiple "thread" "thread" {
> +    -re "Current thread is \($decimal\).*\r\n$gdb_prompt $" {
> +	set thread $expect_out(1,string)
> +    }
> +}
> +
> +# Determine the other thread.
> +set other "bad"
> +if { $thread == 1 } {
> +    set other 2
> +} elseif { $thread == 2 } {
> +    set other 1
> +}
> +
> +# This test only works for scheduler-locking 'replay'.
> +gdb_test_no_output "set scheduler-locking replay"
> +
> +# Remove breakpoints or we might run into it right away.
> +delete_breakpoints
> +
> +# Start replaying the other thread.
> +gdb_test "thread apply $other reverse-stepi" ".*"
> +gdb_test "thread apply $other info record" "Replay in progress.*"
> +
> +# Step the thread that reported the breakpoint, which is not replaying.
> +gdb_test "next" "return arg;.*"
> +
> +proc check_not_replaying { thread } {
> +    global gdb_prompt
> +
> +    gdb_test_multiple "thread apply $thread info record" \
> +	"thread $thread not replaying" {
> +        -re "Replay in progress" {

This inadvertently leaves the prompt in the expect buffer.  You can use -wrap to
match it.

> +            fail $gdb_test_name
> +        }
> +        -re "$gdb_prompt $" {
> +            pass $gdb_test_name
> +        }
> +    }
> +}
> +
> +check_not_replaying 1
> +check_not_replaying 2
> 

Re: [PATCH 0/4] gdb, btrace: infrun fixes

[Pedro Alves]

Hi Markus,

I ran the tests included in the series against pristine / unfixed GDB, and got:

	Using /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
	Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.btrace/stepn.exp ...
	Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp ...
	FAIL: gdb.btrace/implicit-stop-replaying.exp: thread apply 1 info record
	Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.btrace/step-hang.exp ...
	FAIL: gdb.btrace/step-hang.exp: continue until exit (timeout)
	Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.btrace/cont-hang.exp ...
	FAIL: gdb.btrace/cont-hang.exp: continue until exit (timeout)
	Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp ...
	FAIL: gdb.btrace/multi-thread-break-hang.exp: stepi.2 (timeout)
	FAIL: gdb.btrace/multi-thread-break-hang.exp: stepi.3 (timeout)

The curious thing is that stepn.exp doesn't fail, it passes anyhow.  Is that expected?

Also, I diffed gdb.log before/after patch, and noticed a difference.  GDB no longer prints the current
location:

   No more reverse-execution history.
  -main () at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.btrace/record_goto.c:50
   50       return 0;     /* main.3 */

Is this expected?

Thanks,
Pedro Alves

RE: [PATCH 1/4] gdb, infrun, btrace: fix reverse/replay stepping at end of execution history

[Metzger, Markus T]

Thanks for the review, Pedro.

>> That step-over failed after actually completing the step.  This is wrong.
>
>These last two sentences confused me, it is not clear to me what you're
>saying is wrong.  I.e., what exactly does "fail" mean here?  And what exactly
>is wrong?

It is wrong that we complete the step and then fail.  The step itself should
have failed and we should not have completed the step.  I rephrased the
commit message.


>> This exposes another issue, however.  When completing a step-over at the
>> end of the execution history, we implicitly stop replaying that thread.
>
>OOC, where does that happen (implicitly stop replaying)?

In record_btrace_stop_replaying_at_end().


>> A
>> continue command would resume after the step-over and, since we're no
>> longer replaying, would continue recording.
>>
>
>This approach relies on the reverse debugging / replaying being handled by GDB.
>
>I mean, remote targets that support reverse debugging themselves, via the "bc",
>"bs"
>packets (backwards continue/step), and "replaylog" stop reply
>(TARGET_WAITKIND_NO_HISTORY)
>are out of luck since the remote target has no way to implement
>target_record_is_replaying.

Why is that?  The functionality solely depends on the replay state of threads
inside that target.

There's also target_record_will_replay() that takes the stepping command's
execution direction into account.


>Thus I'm wondering whether target_record_is_replaying() existing is a design
>mistake, and
>I wonder -- should it be the target that is guaranteed to reply with no-history
>before
>implicitly stopping replay?  IOW, don't stop replay until a no-history event is
>returned.

We need some mechanism to switch between replay mode and record mode.
E.g. if the current thread is not replaying but other threads are, in all-stop mode
we need to either a) abort the mover command or b) silently stop replaying
other threads.

We could move the decision and hence the code down into the target such
that the move request to the target would fail or silently stop replaying others.

Record-btrace as well as record-full, however, implement replay in GDB and
not in the target.  This keeps gdbserver more light-weight as we wouldn't
need to decode the trace on the target itself, nor store a huge amount of
trace data.  It also saves a lot of round-trips.  I wouldn't change that design.

I don't see, however, why this shouldn't be able to co-exist with record/replay
being implemented on the target side.


> IOW, don't stop replay until a no-history event is returned.

I implicitly stop replaying to make 'rsi, si, cont' work.  Otherwise, the user
would either need to repeatedly continue until he gets no-history or do
'record goto end'.

I also implicitly stop replaying other threads to not have the user iterate
over all threads and do 'record goto end' to stop replaying each thread
before he can resume.


>> Fix that by recording the replay state in the thread's control state and
>> failing with no-history in keep_going if we're switching from replay to
>> recording.
>
>This affects target record just as well, right?  Would it be possible for
>the testcases to be made to test that target as well?

We kept tests separate, so far, since record btrace doesn't support data
and record full doesn't support multi-threading.


>> @@ -7813,6 +7815,26 @@ keep_going_pass_signal (struct
>execution_control_state *ecs)
>>    gdb_assert (ecs->event_thread->ptid == inferior_ptid);
>>    gdb_assert (!ecs->event_thread->resumed);
>>
>> +  /* When a thread reaches the end of its execution history, it automatically
>> +     stops replaying.  This is so the user doesn't need to explicitly stop it
>> +     with a separate command.
>> +
>> +     We do not want a single command (e.g. continue) to transition from
>> +     replaying to recording, though, e.g. when starting from a breakpoint we
>> +     needed to step over at the end of the trace.  When we reach the end of the
>> +     execution history during stepping, stop with no-history.
>> +
>> +     The other direction is fine.  When we're at the end of the execution
>> +     history, we may reverse-continue to start replaying.  */
>> +  if (ecs->event_thread->control.is_replaying
>> +      && !target_record_is_replaying (ecs->event_thread->ptid))
>> +    {
>> +      gdb::observers::no_history.notify ();
>> +      stop_waiting (ecs);
>> +      normal_stop ();
>
>normal_stop looks at the last target status to decide what to do.  So here we
>should call
>set_last_target_status before calling normal_stop, I'd think.

I'm doing that in patch 3.  I moved the code into this patch.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 2/4] gdb, infrun, record: fix hang when step-over fails with no-history

[Metzger, Markus T]

Thanks for the review, Pedro.

>> When trying to step over a breakpoint at the end of the trace while
>> another thread is replaying, the step-over will fail with no-history.
>
>Some information seems missing here.  The no-history event is for the other
>thread that
>is replaying, right?  We don't use displaced stepping when using a record target,
>so why did that
>other thread report the no-history event?  Without displaced stepping, GDB will
>only resume the
>thread stepping over the breakpoint, so I'm confused on how the other thread's
>position
>in the replay log affects the stepping thread.

No, it comes from the thread that was stepped.  Since the other thread is replaying,
we step in the record-btrace target and that fails since the to-be-stepped thread
is at the end of its execution history.  This then causes the hang as described in the
commit message.


>> This does not clear step_over_info so a subsequent resume will cause GDB
>> to not resume the thread and expect a SIGTRAP to complete the step-over.
>> This will never come causing GDB to hang in the wait-for-event poll.
>>
>> This is a variant of the issue fixed in the parent commit.  That commit
>> addressed the issue for a single-threaded process and fixed an issue with
>> reverse/replay stepping in general.
>>
>> This commit addresses the issue for a multi-threaded process.  In this
>> case, the single-step does not complete.
>>
>> Finish an in-flight step-over when a thread stopped with NO_HISTORY.
>> Since we did not move, we will simply start the step-over again.


>> +set thread "bad"
>> +gdb_test_multiple "thread" "thread" {
>> +    -re "Current thread is \($decimal\).*\r\n$gdb_prompt $" {
>> +	set thread $expect_out(1,string)
>> +    }
>> +}
>
>This is fine, but FYI, it could be done with a single liner:
>
>  set thread [get_integer_valueof "\$_thread" bad]

Thanks, that's indeed nicer.


>> +# Determine the other thread.
>> +set other "bad"
>> +if { $thread == 1 } {
>> +    set other 2
>> +} elseif { $thread == 2 } {
>> +    set other 1
>> +}
>> +
>> +# This test works for scheduler-locking 'on' or 'step'; 'replay' would
>> +# implicitly stop replaying, avoiding the problem; 'off' would step one
>> +# and resume the other.  The test would work for the current lock-step
>> +# implementation.
>
>What does the last sentence about "lock-step" mean?  Also, I find "work"
>ambiguous.
>I take it here is means "exercises the original bug".  How about saying that?

The term 'lock-step' refers to the stepping implementation in record-btrace
where we single-step resumed threads one-by-one in lock-step.  We don't
want to bake that behavior into the test.

Rephrased the comment.


>> +gdb_test_no_output "set scheduler-locking step"
>> +
>> +# Start replaying the other thread.  This will prevent stepping the thread
>> +# that reported the event.
>
>What is the "this" in "This will prevent"?  Is it the use of "record goto" you're
>referring to, or something else?

'This' is referring to the preceding sentence.  By replaying one thread, we
prevent stepping the other thread.


>> +gdb_test "thread apply $other record goto begin" ".*"
>> +gdb_test "thread apply $other info record" "Replay in progress.*"
>> +
>> +# We're at a breakpoint so this triggers step-over.  Since we're at the
>> +# end of the trace, the step will fail.
>
>What is "we" here?  It's the "other" thread, not the stepping thread, right?

No, it's this thread.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 4/4] gdb, infrun: fix multi-threaded reverse stepping

[Metzger, Markus T]

Thanks for the review, Pedro.

>> I would have expected everything to be completed by the time the infcmd
>> function returns but I cannot say whether the behaviour I'm seeing is
>> intentional.
>
>The going via the event loop to handle the pending event is intentional,
>simplifies
>things by only having one code path.

Thanks for clarifying.  I rephrased the commit message.


>> Assuming it is, this patch addresses the loss of the execution direction
>> by storing the direction in a thread's control state and changing most of
>> infrun to take it from there rather than using the global variable.
>
>Do you envision that you'll support having some threads moving forward and
>other threads moving backwards, all in the same inferior or target?
>
>Like:
>
> (gdb) thread 1.1
> (gdb) c&               // thread 1.1 is now continuously recording
> (gdb) thread 1.2
> (gdb) reverse-continue // thread 1.2 is running backwards

The current implementation does not allow that but the interface would.


>Which makes me question -- seems odd to have both the direction recorded in
>all the threads, and then still have target_execution_direction() ?
>
>Do we want to eliminate one of these?  Either the target direction, or the threads
>direction?
>
>If we don't want to support threads of the same target executing in different
>directions,
>then I suspect that we can reimplement this patch by adding a new
>target_set_execution_direction
>target method, that would be called in the case where infrun skips calling
>target_resume
>because of a pending event.

We already allow one thread to move forward and another to move backward
in non-stop mode.  What we do not allow is one thread to continue recording
while another is replaying.

This works since commands are issued sequentially and the execution direction
is toggled between commands.  When a thread is actually resumed, record-btrace
stores the execution direction as part of the stepping command:

enum btrace_thread_flag : unsigned
{
  /* The thread is to be stepped forwards.  */
  BTHR_STEP = (1 << 0),

  /* The thread is to be stepped backwards.  */
  BTHR_RSTEP = (1 << 1),

  /* The thread is to be continued forwards.  */
  BTHR_CONT = (1 << 2),

  /* The thread is to be continued backwards.  */
  BTHR_RCONT = (1 << 3),

  /* The thread is to be moved.  */
  BTHR_MOVE = (BTHR_STEP | BTHR_RSTEP | BTHR_CONT | BTHR_RCONT),

  /* The thread is to be stopped.  */
  BTHR_STOP = (1 << 4)
};

In the scenario addressed by this patch, there were separate resume requests
for the same thread to record-btrace between which the direction got lost.


>If we do want to support mixed directions, then is the following snipped from
>fetch_inferior_event still useful for anything?
>
>    scoped_restore save_exec_dir
>      = make_scoped_restore (&execution_direction,
>			     target_execution_direction ());
>
>AFAICS, when handling an event we'll now use the thread's execution direction,
>so flipping
>the global like this here is basically dead code.

Hmmm, it probably is indeed redundant.


>> @@ -7404,6 +7416,11 @@ keep_going_stepped_thread (struct thread_info *tp)
>>
>>        tp->resumed = true;
>>        resume_ptid = internal_resume_ptid (tp->control.stepping_command);
>> +
>> +      scoped_restore save_exec_dir
>> +	= make_scoped_restore (&execution_direction,
>> +			       tp->control.execution_direction);
>
>Shouldn't this be done for all do_target_resume calls?  How about doing it from
>within do_target_resume instead?

Sounds good.


>> +    gdb_test_multiple "thread apply $thread info record" \
>> +	"thread $thread not replaying" {
>> +        -re "Replay in progress" {
>
>This inadvertently leaves the prompt in the expect buffer.  You can use -wrap to
>match it.

Thanks.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 0/4] gdb, btrace: infrun fixes

[Metzger, Markus T]

Hello Pedro,

>The curious thing is that stepn.exp doesn't fail, it passes anyhow.  Is that
>expected?

I added stepn for completeness.


>Also, I diffed gdb.log before/after patch, and noticed a difference.  GDB no longer
>prints the current
>location:
>
>   No more reverse-execution history.
>  -main () at /home/pedro/gdb/binutils-
>gdb/src/gdb/testsuite/gdb.btrace/record_goto.c:50
>   50       return 0;     /* main.3 */
>
>Is this expected?

Thanks for pointing this out, I hadn't noticed it.

The reason is that with the patch, we now actually complete the 1st of the 5 stepi.
Before, although we actually completed the step, we signaled no-history.  On the 2nd
step, we signal no-history in keep_going() since the thread is replaying but the target
no longer is.

Whether we print the frame is guarded by tp->control.step_stop.  We clear it in
clear_proceed_status_thread() initially and then in handle_signal_stop() after we
handled breakpoint stop events.  So, the 1 from end_stepping_range() during the
1st stop is preserved.

I'm wondering if prepare_one_step() should reset tp->control.step_stop to zero.  It does
re-initialize tp->control.step_range_start/end and we do start another step.

What do you think?

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928