[PATCH v2 0/6] fix PR gdb/19340

[PATCH v2 0/6] fix PR gdb/19340

[Markus Metzger]

Fix PR gdb/19340 by stopping replaying the current thread before disabling
recording.

While writing the test for this, I wondered whether it would also work if
there was more than one process and if some were recording while others
were replaying.

It turned out that recording one process while some other process is
replaying doesn't work.  You first have to stop replaying everything
before you can continue recording.  Since btrace is per inferior, this
seems like an unnecessary restriction.

The first patch fixes gdb/pr19340; the rest makes recording work together
with replaying on inferior level.

The most prominent change is probably that do_target_wait() no longer
silently switches inferiors.  This infrun change requires global
maintainer approval.

Changes to v1:
  - fixed a fail in a newly added test
  - fixed fails in patch 5, which also caused the patch to get renamed

Markus Metzger (6):
  gdb, btrace: fix pr19340
  gdb, btrace: simplify gdb.btrace/multi-inferior.exp
  gdb, btrace: remove record_btrace_target::supports_*()
  gdb, btrace: set wait status to ignore if nothing is moving
  gdb, infrun: fix silent inferior switch in do_target_wait()
  gdb, btrace, infrun: per-inferior run-control

 gdb/infrun.c                                | 63 ++++++++++++++---
 gdb/linux-nat.c                             | 17 +++--
 gdb/record-btrace.c                         | 78 +++++++--------------
 gdb/remote.c                                | 22 ++++--
 gdb/testsuite/gdb.btrace/multi-inferior.c   | 10 ++-
 gdb/testsuite/gdb.btrace/multi-inferior.exp | 33 ++++-----
 gdb/testsuite/gdb.btrace/step.exp           | 22 ++++--
 7 files changed, 150 insertions(+), 95 deletions(-)

-- 
2.34.1

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 v2 1/6] gdb, btrace: fix pr19340

[Markus Metzger]

GDB fails with an assertion when stopping recording on a replaying thread
and then resuming that thread.  Stopping recording left the thread
replaying but the record target is gone.

Stop replaying all threads in the selected inferior before stopping recording.

If the selected thread had been replaying, print the (potentially updated)
location.

I had to change the stepping test slightly to account for different
compilers generating slightly different debug information, so when
stepping the 'return 0' after 'record stop' I would end up in a different
location depending on which compiler I used.  The test still covers all
stepping commands.

Fixes PR gdb/19340.
---
 gdb/record-btrace.c               | 17 +++++++++++++++++
 gdb/testsuite/gdb.btrace/step.exp | 22 +++++++++++++++-------
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 0ef1dfbe652..1d946f9d3f2 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -414,11 +414,28 @@ record_btrace_target::stop_recording ()
 {
   DEBUG ("stop recording");
 
+  /* Remember the selected thread if it exists and is replaying.  */
+  thread_info *curr = nullptr;
+  if (inferior_ptid.lwp_p ())
+    {
+      curr = inferior_thread ();
+      if (!btrace_is_replaying (curr))
+	curr = nullptr;
+    }
+
+  record_stop_replaying ();
   record_btrace_auto_disable ();
 
   for (thread_info *tp : current_inferior ()->non_exited_threads ())
     if (tp->btrace.target != NULL)
       btrace_disable (tp);
+
+  /* Print the updated location in case we had stopped a replaying thread.  */
+  if (curr != nullptr)
+    {
+      curr->set_stop_pc (regcache_read_pc (get_thread_regcache (curr)));
+      print_stack_frame (get_selected_frame (), 1, SRC_AND_LOC, 1);
+    }
 }
 
 /* The disconnect method of target record-btrace.  */
diff --git a/gdb/testsuite/gdb.btrace/step.exp b/gdb/testsuite/gdb.btrace/step.exp
index 0adc18bd924..ed6d2140bdd 100644
--- a/gdb/testsuite/gdb.btrace/step.exp
+++ b/gdb/testsuite/gdb.btrace/step.exp
@@ -31,18 +31,26 @@ if ![runto_main] {
 # trace the call to the test function
 with_test_prefix "record" {
     gdb_test_no_output "record btrace"
-    gdb_test "next"
+    gdb_test "step"
+    gdb_test "next 2"
 }
 
 # let's step around a bit
 with_test_prefix "replay" {
-    gdb_test "reverse-next" ".*main\.2.*" "reverse-next.1"
+    gdb_test "reverse-finish" ".*main\.2.*" "reverse-finish.1"
     gdb_test "step" ".*fun4\.2.*" "step.1"
     gdb_test "next" ".*fun4\.3.*" "next.1"
     gdb_test "step" ".*fun2\.2.*" "step.2"
-    gdb_test "finish" ".*fun4\.4.*" "finish.1"
-    gdb_test "reverse-step" ".*fun2\.3.*" "reverse-step.1"
-    gdb_test "reverse-finish" ".*fun4\.3.*" "reverse-finish.1"
-    gdb_test "reverse-next" ".*fun4\.2.*" "reverse-next.2"
-    gdb_test "reverse-finish" ".*main\.2.*" "reverse-finish.2"
+    gdb_test "reverse-finish" ".*fun4\.3.*" "reverse-finish.2"
+    gdb_test "reverse-step" ".*fun1\.2.*" "reverse-step.1"
+    gdb_test "finish" ".*fun4\.3.*" "finish.1"
+    gdb_test "reverse-next" ".*fun4\.2.*" "reverse-next.1"
+    gdb_test "reverse-finish" ".*main\.2.*" "reverse-finish.3"
+}
+
+# stop recording and try to step live (pr19340)
+with_test_prefix "live" {
+    gdb_test "record stop" "fun4\.4.*Process record is stopped.*"
+    gdb_test "reverse-next" "Target .* does not support this command.*"
+    gdb_test "step" ".*fun3\.2.*"
 }
-- 
2.34.1

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 v2 2/6] gdb, btrace: simplify gdb.btrace/multi-inferior.exp

[Markus Metzger]

We don't really need three inferiors to test multi-inferior recording.
We don't really need to start recording on the second inferior first.
We don't really need to check info record before starting recording.
If we were recording, there would be output, causing a fail.

This just complicates the test when there is something to debug.
---
 gdb/testsuite/gdb.btrace/multi-inferior.exp | 24 +++------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/gdb/testsuite/gdb.btrace/multi-inferior.exp b/gdb/testsuite/gdb.btrace/multi-inferior.exp
index 6996b182e65..174d38364a4 100644
--- a/gdb/testsuite/gdb.btrace/multi-inferior.exp
+++ b/gdb/testsuite/gdb.btrace/multi-inferior.exp
@@ -37,6 +37,8 @@ with_test_prefix "inferior 1" {
     if ![runto_main] {
 	return -1
     }
+
+    gdb_test_no_output "record btrace"
 }
 
 with_test_prefix "inferior 2" {
@@ -48,25 +50,5 @@ with_test_prefix "inferior 2" {
 	return -1
     }
 
-    gdb_test_no_output "record btrace" "record btrace"
-}
-
-with_test_prefix "inferior 1" {
-    gdb_test "inferior 1" "Switching to inferior 1.*"
-
-    gdb_test "info record" "No recording is currently active\\."
-    gdb_test_no_output "record btrace" "record btrace"
-}
-
-with_test_prefix "inferior 3" {
-    gdb_test "add-inferior -exec ${host_binfile}" "Added inferior 3.*" \
-	"add third inferior"
-    gdb_test "inferior 3" "Switching to inferior 3.*"
-
-    if ![runto_main] {
-	return -1
-    }
-
-    gdb_test "info record" "No recording is currently active\\."
-    gdb_test_no_output "record btrace" "record btrace"
+    gdb_test_no_output "record btrace"
 }
-- 
2.34.1

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 v2 3/6] gdb, btrace: remove record_btrace_target::supports_*()

[Markus Metzger]

Let's not introduce support for breakpoint types the target beneath does
not support, even though we could while replaying.

Otherwise, users may set breakpoints during replay that then couldn't be
inserted into the target when switching back to recording.
---
 gdb/record-btrace.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 1d946f9d3f2..66f283b8680 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -130,10 +130,7 @@ class record_btrace_target final : public target_ops
   bool can_execute_reverse () override;
 
   bool stopped_by_sw_breakpoint () override;
-  bool supports_stopped_by_sw_breakpoint () override;
-
   bool stopped_by_hw_breakpoint () override;
-  bool supports_stopped_by_hw_breakpoint () override;
 
   enum exec_direction_kind execution_direction () override;
   void prepare_to_generate_core () override;
@@ -2706,18 +2703,6 @@ record_btrace_target::stopped_by_sw_breakpoint ()
   return this->beneath ()->stopped_by_sw_breakpoint ();
 }
 
-/* The supports_stopped_by_sw_breakpoint method of target
-   record-btrace.  */
-
-bool
-record_btrace_target::supports_stopped_by_sw_breakpoint ()
-{
-  if (record_is_replaying (minus_one_ptid))
-    return true;
-
-  return this->beneath ()->supports_stopped_by_sw_breakpoint ();
-}
-
 /* The stopped_by_sw_breakpoint method of target record-btrace.  */
 
 bool
@@ -2733,18 +2718,6 @@ record_btrace_target::stopped_by_hw_breakpoint ()
   return this->beneath ()->stopped_by_hw_breakpoint ();
 }
 
-/* The supports_stopped_by_hw_breakpoint method of target
-   record-btrace.  */
-
-bool
-record_btrace_target::supports_stopped_by_hw_breakpoint ()
-{
-  if (record_is_replaying (minus_one_ptid))
-    return true;
-
-  return this->beneath ()->supports_stopped_by_hw_breakpoint ();
-}
-
 /* The update_thread_list method of target record-btrace.  */
 
 void
-- 
2.34.1

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 v2 4/6] gdb, btrace: set wait status to ignore if nothing is moving

[Markus Metzger]

When record_btrace::wait() is called and no threads are moving, we set the
wait status to no_resumed.  Change that to ignore.

This helps with enabling per-inferior run-control for the record btrace
target as it avoids breaking out of do_target_wait() too early with
no_resumed when there would have been an event on another thread.
---
 gdb/record-btrace.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 66f283b8680..53ed481eb4d 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2279,14 +2279,14 @@ btrace_step_spurious (void)
   return status;
 }
 
-/* Return a target_waitstatus indicating that the thread was not resumed.  */
+/* Return a target_waitstatus indicating that nothing is moving.  */
 
 static struct target_waitstatus
-btrace_step_no_resumed (void)
+btrace_step_no_moving_threads (void)
 {
   struct target_waitstatus status;
 
-  status.set_no_resumed ();
+  status.set_ignore ();
 
   return status;
 }
@@ -2555,7 +2555,7 @@ record_btrace_target::wait (ptid_t ptid, struct target_waitstatus *status,
 
   if (moving.empty ())
     {
-      *status = btrace_step_no_resumed ();
+      *status = btrace_step_no_moving_threads ();
 
       DEBUG ("wait ended by %s: %s", null_ptid.to_string ().c_str (),
 	     status->to_string ().c_str ());
-- 
2.34.1

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 v2 5/6] gdb, infrun: fix silent inferior switch in do_target_wait()

[Markus Metzger]

In do_target_wait(), we iterate over inferiors and call
do_target_wait_1(), which eventually calls target_wait() per inferior.
Each time, we wait for minus_one_ptid.

In some cases, e.g. gdb.threads/detach-step-over.exp, we ask to wait for
one inferior, and get an event from a different inferior back without
noticing the inferior switch.

Wait for a single inferior, instead.  Since we iterate over all inferiors,
we still cover everything.

This exposes another bug with STOP_QUIETLY_NO_SIGSTOP handling.

After attaching, we interrupt all threads in the new inferior, then call
do_target_wait() to receive the stopped events.  This randomly selects an
inferior to start waiting for and iterates over all inferiors starting
from there.

The initial stop event for the main thread is already queued up, so we
wouldn't actually wait() if we had started with the new inferior.  Or if
we had waited for minus_one_ptid, which would then have silently switched
inferiors.

Since we no longer allow that, we may actually wait() for the new inferior
and find other events to report, out of which we randomly select one.

If we selected an event for another thread, e.g. one that had been
interrupted as part of non-stop attach, STOP_QUIETLY_NO_SIGSTOP would be
applied to that thread (unnecessarily), leaving the main thread with a
SIGSTOP event but last_resume_kind = 0 (resume_continue).

When the main thread is later selected, SIGSTOP is reported to the user.

Normally, linux-nat's wait() turns the SIGSTOP it uses for interrupting
threads into GDB_SIGNAL_0.  This is based on last_resume_kind, which is
set to 2 (resume_stop) when sending SIGSTOP to interrupt a thread.

We do this for all threads of the new inferior when interrupting them as
part of non-stop attach.  Except for the main thread, which we expect to
be reported before the first wait().

Set last_resume_kind to resume_stop for the main thread after attaching.
---
 gdb/infrun.c    | 46 +++++++++++++++++++++++++++++++++++++++++++---
 gdb/linux-nat.c | 17 ++++++++++++-----
 gdb/remote.c    | 22 +++++++++++++++++-----
 3 files changed, 72 insertions(+), 13 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index a5030b16376..9ca0571065c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4198,7 +4198,23 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
 
   auto do_wait = [&] (inferior *inf)
   {
-    ecs->ptid = do_target_wait_1 (inf, wait_ptid, &ecs->ws, options);
+    ptid_t ptid { inf->pid };
+
+    /* Make sure we're not widening WAIT_PTID.  */
+    if (!ptid.matches (wait_ptid)
+	/* Targets that cannot async will be asked for a blocking wait.
+
+	   Blocking wait does not work inferior-by-inferior if the target
+	   provides more than one inferior.  Fall back to waiting for
+	   WAIT_PTID in that case.  */
+	|| !target_can_async_p () || ((options & TARGET_WNOHANG) == 0)
+	/* FIXME: I don't see why we should have inferiors with zero pid,
+	   which indicates that the respective ptid is not a process.
+	   They do exist, though, and we cannot wait for them.  */
+	|| !ptid.is_pid ())
+      ptid = wait_ptid;
+
+    ecs->ptid = do_target_wait_1 (inf, ptid, &ecs->ws, options);
     ecs->target = inf->process_target ();
     return (ecs->ws.kind () != TARGET_WAITKIND_IGNORE);
   };
@@ -4208,6 +4224,12 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
      reported the stop to the user, polling for events.  */
   scoped_restore_current_thread restore_thread;
 
+  /* The first TARGET_WAITKIND_NO_RESUMED execution state.
+
+     If we do not find a more interesting event, we will report that.  */
+  execution_control_state no_resumed {};
+  no_resumed.ptid = null_ptid;
+
   intrusive_list_iterator<inferior> start
     = inferior_list.iterator_to (*selected);
 
@@ -4218,7 +4240,13 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
       inferior *inf = &*it;
 
       if (inferior_matches (inf) && do_wait (inf))
-	return true;
+	{
+	  if (ecs->ws.kind () != TARGET_WAITKIND_NO_RESUMED)
+	    return true;
+
+	  if (no_resumed.ptid == null_ptid)
+	    no_resumed = *ecs;
+	}
     }
 
   for (intrusive_list_iterator<inferior> it = inferior_list.begin ();
@@ -4228,7 +4256,19 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
       inferior *inf = &*it;
 
       if (inferior_matches (inf) && do_wait (inf))
-	return true;
+	{
+	  if (ecs->ws.kind () != TARGET_WAITKIND_NO_RESUMED)
+	    return true;
+
+	  if (no_resumed.ptid == null_ptid)
+	    no_resumed = *ecs;
+	}
+    }
+
+  if (no_resumed.ptid != null_ptid)
+    {
+      *ecs = no_resumed;
+      return true;
     }
 
   ecs->ws.set_ignore ();
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 2602e1f240d..06b39d67a72 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1154,6 +1154,7 @@ linux_nat_target::attach (const char *args, int from_tty)
 
   /* Add the initial process as the first LWP to the list.  */
   lp = add_initial_lwp (ptid);
+  lp->last_resume_kind = resume_stop;
 
   status = linux_nat_post_attach_wait (lp->ptid, &lp->signalled);
   if (!WIFSTOPPED (status))
@@ -3329,12 +3330,18 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
      moment at which we know its PID.  */
   if (ptid.is_pid () && find_lwp_pid (ptid) == nullptr)
     {
-      ptid_t lwp_ptid (ptid.pid (), ptid.pid ());
+      /* Unless we already did and this is simply a request to wait for a
+	 particular inferior.  */
+      inferior *inf = find_inferior_ptid (linux_target, ptid);
+      if (inf && inf->find_thread (ptid))
+	{
+	  ptid_t lwp_ptid (ptid.pid (), ptid.pid ());
 
-      /* Upgrade the main thread's ptid.  */
-      thread_change_ptid (linux_target, ptid, lwp_ptid);
-      lp = add_initial_lwp (lwp_ptid);
-      lp->resumed = 1;
+	  /* Upgrade the main thread's ptid.  */
+	  thread_change_ptid (linux_target, ptid, lwp_ptid);
+	  lp = add_initial_lwp (lwp_ptid);
+	  lp->resumed = 1;
+	}
     }
 
   /* Make sure SIGCHLD is blocked until the sigsuspend below.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index a09ba4d715d..49abd4e4376 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7825,12 +7825,24 @@ remote_target::remote_notif_remove_queued_reply (ptid_t ptid)
 {
   remote_state *rs = get_remote_state ();
 
+  auto pred = [=] (const stop_reply_up &event)
+  {
+    /* A null ptid should only happen if we have a single process.  It
+       wouldn't match the process ptid, though, so let's check this case
+       separately.  */
+    if ((event->ptid == null_ptid) && ptid.is_pid ())
+      return true;
+
+    /* A minus one ptid should only happen for events that match
+       everything.  It wouldn't match a process or thread ptid, though, so
+       let's check this case separately.  */
+    if (event->ptid == minus_one_ptid)
+      return true;
+
+    return event->ptid.matches (ptid);
+  };
   auto iter = std::find_if (rs->stop_reply_queue.begin (),
-			    rs->stop_reply_queue.end (),
-			    [=] (const stop_reply_up &event)
-			    {
-			      return event->ptid.matches (ptid);
-			    });
+			    rs->stop_reply_queue.end (), pred);
   stop_reply_up result;
   if (iter != rs->stop_reply_queue.end ())
     {
-- 
2.34.1

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 v2 6/6] gdb, btrace, infrun: per-inferior run-control

[Markus Metzger]

While recording is already per inferior, run-control isn't.  As soon as
any thread in any inferior is replaying, no other inferior can be resumed.

This is controlled by many calls to record_is_replaying(minus_one_ptid).
Instead of minus_one_ptid, pass the ptid of the inferior to be checked.
---
 gdb/infrun.c                                | 17 ++++++++------
 gdb/record-btrace.c                         | 26 +++++----------------
 gdb/testsuite/gdb.btrace/multi-inferior.c   | 10 +++++++-
 gdb/testsuite/gdb.btrace/multi-inferior.exp | 19 +++++++++++++++
 4 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 9ca0571065c..a237d970030 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2399,7 +2399,8 @@ user_visible_resume_ptid (int step)
       resume_ptid = inferior_ptid;
     }
   else if ((scheduler_mode == schedlock_replay)
-	   && target_record_will_replay (minus_one_ptid, execution_direction))
+	   && target_record_will_replay (ptid_t (inferior_ptid.pid ()),
+					 execution_direction))
     {
       /* User-settable 'scheduler' mode requires solo thread resume in replay
 	 mode.  */
@@ -3117,15 +3118,17 @@ clear_proceed_status (int step)
      This is a convenience feature to not require the user to explicitly
      stop replaying the other threads.  We're assuming that the user's
      intent is to resume tracing the recorded process.  */
+  ptid_t resume_ptid = user_visible_resume_ptid (step);
   if (!non_stop && scheduler_mode == schedlock_replay
-      && target_record_is_replaying (minus_one_ptid)
-      && !target_record_will_replay (user_visible_resume_ptid (step),
-				     execution_direction))
-    target_record_stop_replaying ();
+      && target_record_is_replaying (ptid_t (resume_ptid.pid ()))
+      && !target_record_will_replay (resume_ptid, execution_direction))
+    {
+      target_record_stop_replaying ();
+      resume_ptid = user_visible_resume_ptid (step);
+    }
 
   if (!non_stop && inferior_ptid != null_ptid)
     {
-      ptid_t resume_ptid = user_visible_resume_ptid (step);
       process_stratum_target *resume_target
 	= user_visible_resume_target (resume_ptid);
 
@@ -3204,7 +3207,7 @@ schedlock_applies (struct thread_info *tp)
 	  || (scheduler_mode == schedlock_step
 	      && tp->control.stepping_command)
 	  || (scheduler_mode == schedlock_replay
-	      && target_record_will_replay (minus_one_ptid,
+	      && target_record_will_replay (ptid_t (tp->inf->pid),
 					    execution_direction)));
 }
 
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 53ed481eb4d..3f9e19b1a02 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -121,7 +121,6 @@ class record_btrace_target final : public target_ops
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
 
   void stop (ptid_t) override;
-  void update_thread_list () override;
   bool thread_alive (ptid_t ptid) override;
   void goto_record_begin () override;
   void goto_record_end () override;
@@ -2159,7 +2158,7 @@ record_btrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
      make progress, we may need to explicitly move replaying threads to the end
      of their execution history.  */
   if ((::execution_direction != EXEC_REVERSE)
-      && !record_is_replaying (minus_one_ptid))
+      && !record_is_replaying (ptid_t (ptid.pid ())))
     {
       this->beneath ()->resume (ptid, step, signal);
       return;
@@ -2542,7 +2541,7 @@ record_btrace_target::wait (ptid_t ptid, struct target_waitstatus *status,
 
   /* As long as we're not replaying, just forward the request.  */
   if ((::execution_direction != EXEC_REVERSE)
-      && !record_is_replaying (minus_one_ptid))
+      && !record_is_replaying (ptid_t (ptid.pid ())))
     {
       return this->beneath ()->wait (ptid, status, options);
     }
@@ -2663,7 +2662,7 @@ record_btrace_target::stop (ptid_t ptid)
 
   /* As long as we're not replaying, just forward the request.  */
   if ((::execution_direction != EXEC_REVERSE)
-      && !record_is_replaying (minus_one_ptid))
+      && !record_is_replaying (ptid_t (ptid.pid ())))
     {
       this->beneath ()->stop (ptid);
     }
@@ -2693,7 +2692,7 @@ record_btrace_target::can_execute_reverse ()
 bool
 record_btrace_target::stopped_by_sw_breakpoint ()
 {
-  if (record_is_replaying (minus_one_ptid))
+  if (record_is_replaying (ptid_t (inferior_ptid.pid ())))
     {
       struct thread_info *tp = inferior_thread ();
 
@@ -2708,7 +2707,7 @@ record_btrace_target::stopped_by_sw_breakpoint ()
 bool
 record_btrace_target::stopped_by_hw_breakpoint ()
 {
-  if (record_is_replaying (minus_one_ptid))
+  if (record_is_replaying (ptid_t (inferior_ptid.pid ())))
     {
       struct thread_info *tp = inferior_thread ();
 
@@ -2718,26 +2717,13 @@ record_btrace_target::stopped_by_hw_breakpoint ()
   return this->beneath ()->stopped_by_hw_breakpoint ();
 }
 
-/* The update_thread_list method of target record-btrace.  */
-
-void
-record_btrace_target::update_thread_list ()
-{
-  /* We don't add or remove threads during replay.  */
-  if (record_is_replaying (minus_one_ptid))
-    return;
-
-  /* Forward the request.  */
-  this->beneath ()->update_thread_list ();
-}
-
 /* The thread_alive method of target record-btrace.  */
 
 bool
 record_btrace_target::thread_alive (ptid_t ptid)
 {
   /* We don't add or remove threads during replay.  */
-  if (record_is_replaying (minus_one_ptid))
+  if (record_is_replaying (ptid_t (ptid.pid ())))
     return true;
 
   /* Forward the request.  */
diff --git a/gdb/testsuite/gdb.btrace/multi-inferior.c b/gdb/testsuite/gdb.btrace/multi-inferior.c
index fb4ffc22a17..6f1052a7f25 100644
--- a/gdb/testsuite/gdb.btrace/multi-inferior.c
+++ b/gdb/testsuite/gdb.btrace/multi-inferior.c
@@ -15,8 +15,16 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+static int
+fun (void)
+{
+  int x = fun (); /* fun.1 */
+  return x;       /* fun.2 */
+}
+
 int
 main (void)
 {
-  return 0;
+  int x = fun (); /* main.1 */
+  return x;       /* main.2 */
 }
diff --git a/gdb/testsuite/gdb.btrace/multi-inferior.exp b/gdb/testsuite/gdb.btrace/multi-inferior.exp
index 174d38364a4..df7f423a088 100644
--- a/gdb/testsuite/gdb.btrace/multi-inferior.exp
+++ b/gdb/testsuite/gdb.btrace/multi-inferior.exp
@@ -39,6 +39,8 @@ with_test_prefix "inferior 1" {
     }
 
     gdb_test_no_output "record btrace"
+    gdb_test "step 4" "fun\.1.*"
+    gdb_test "reverse-step" "fun\.1.*"
 }
 
 with_test_prefix "inferior 2" {
@@ -51,4 +53,21 @@ with_test_prefix "inferior 2" {
     }
 
     gdb_test_no_output "record btrace"
+    gdb_test "step 4" "fun\.1.*"
+    gdb_test "reverse-step" "fun\.1.*"
+
+    gdb_test "info record" "Replay in progress.*"
+    gdb_test "record stop" "Process record is stopped.*"
+
+    gdb_test "step" "fun\.1.*"
+}
+
+with_test_prefix "inferior 1" {
+    gdb_test "inferior 1" "Switching to inferior 1.*"
+
+    gdb_test "info record" "Replay in progress.*"
+    gdb_test "reverse-finish" "fun\.1.*"
+    gdb_test "record goto end" "fun\.1.*"
+    gdb_test "step 2" "fun\.1.*"
+    gdb_test "reverse-step 3"
 }
-- 
2.34.1

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

[PING] [PATCH v2 0/6] fix PR gdb/19340

[Metzger, Markus T]

Kindly pinging.

Markus.

>-----Original Message-----
>From: Metzger, Markus T <markus.t.metzger@intel.com>
>Sent: Thursday, April 11, 2024 7:26 AM
>To: gdb-patches@sourceware.org
>Cc: pedro@palves.net
>Subject: [PATCH v2 0/6] fix PR gdb/19340
>
>Fix PR gdb/19340 by stopping replaying the current thread before disabling
>recording.
>
>While writing the test for this, I wondered whether it would also work if
>there was more than one process and if some were recording while others
>were replaying.
>
>It turned out that recording one process while some other process is
>replaying doesn't work.  You first have to stop replaying everything
>before you can continue recording.  Since btrace is per inferior, this
>seems like an unnecessary restriction.
>
>The first patch fixes gdb/pr19340; the rest makes recording work together
>with replaying on inferior level.
>
>The most prominent change is probably that do_target_wait() no longer
>silently switches inferiors.  This infrun change requires global
>maintainer approval.
>
>Changes to v1:
>  - fixed a fail in a newly added test
>  - fixed fails in patch 5, which also caused the patch to get renamed
>
>Markus Metzger (6):
>  gdb, btrace: fix pr19340
>  gdb, btrace: simplify gdb.btrace/multi-inferior.exp
>  gdb, btrace: remove record_btrace_target::supports_*()
>  gdb, btrace: set wait status to ignore if nothing is moving
>  gdb, infrun: fix silent inferior switch in do_target_wait()
>  gdb, btrace, infrun: per-inferior run-control
>
> gdb/infrun.c                                | 63 ++++++++++++++---
> gdb/linux-nat.c                             | 17 +++--
> gdb/record-btrace.c                         | 78 +++++++--------------
> gdb/remote.c                                | 22 ++++--
> gdb/testsuite/gdb.btrace/multi-inferior.c   | 10 ++-
> gdb/testsuite/gdb.btrace/multi-inferior.exp | 33 ++++-----
> gdb/testsuite/gdb.btrace/step.exp           | 22 ++++--
> 7 files changed, 150 insertions(+), 95 deletions(-)
>
>--
>2.34.1

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

[PING v2] [PATCH v2 0/6] fix PR gdb/19340

[Metzger, Markus T]

Kindly pinging.

Markus.

>-----Original Message-----
>From: Metzger, Markus T <markus.t.metzger@intel.com>
>Sent: Friday, May 3, 2024 7:25 AM
>To: gdb-patches@sourceware.org
>Cc: pedro@palves.net
>Subject: [PING] [PATCH v2 0/6] fix PR gdb/19340
>
>Kindly pinging.
>
>Markus.
>
>>-----Original Message-----
>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>Sent: Thursday, April 11, 2024 7:26 AM
>>To: gdb-patches@sourceware.org
>>Cc: pedro@palves.net
>>Subject: [PATCH v2 0/6] fix PR gdb/19340
>>
>>Fix PR gdb/19340 by stopping replaying the current thread before disabling
>>recording.
>>
>>While writing the test for this, I wondered whether it would also work if
>>there was more than one process and if some were recording while others
>>were replaying.
>>
>>It turned out that recording one process while some other process is
>>replaying doesn't work.  You first have to stop replaying everything
>>before you can continue recording.  Since btrace is per inferior, this
>>seems like an unnecessary restriction.
>>
>>The first patch fixes gdb/pr19340; the rest makes recording work together
>>with replaying on inferior level.
>>
>>The most prominent change is probably that do_target_wait() no longer
>>silently switches inferiors.  This infrun change requires global
>>maintainer approval.
>>
>>Changes to v1:
>>  - fixed a fail in a newly added test
>>  - fixed fails in patch 5, which also caused the patch to get renamed
>>
>>Markus Metzger (6):
>>  gdb, btrace: fix pr19340
>>  gdb, btrace: simplify gdb.btrace/multi-inferior.exp
>>  gdb, btrace: remove record_btrace_target::supports_*()
>>  gdb, btrace: set wait status to ignore if nothing is moving
>>  gdb, infrun: fix silent inferior switch in do_target_wait()
>>  gdb, btrace, infrun: per-inferior run-control
>>
>> gdb/infrun.c                                | 63 ++++++++++++++---
>> gdb/linux-nat.c                             | 17 +++--
>> gdb/record-btrace.c                         | 78 +++++++--------------
>> gdb/remote.c                                | 22 ++++--
>> gdb/testsuite/gdb.btrace/multi-inferior.c   | 10 ++-
>> gdb/testsuite/gdb.btrace/multi-inferior.exp | 33 ++++-----
>> gdb/testsuite/gdb.btrace/step.exp           | 22 ++++--
>> 7 files changed, 150 insertions(+), 95 deletions(-)
>>
>>--
>>2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PING v3] [PATCH v2 0/6] fix PR gdb/19340

[Metzger, Markus T]

Kindly pinging.

Markus.

>-----Original Message-----
>From: Metzger, Markus T <markus.t.metzger@intel.com>
>Sent: Friday, June 7, 2024 7:07 AM
>To: gdb-patches@sourceware.org
>Subject: [PING v2] [PATCH v2 0/6] fix PR gdb/19340
>
>Kindly pinging.
>
>Markus.
>
>>-----Original Message-----
>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>Sent: Friday, May 3, 2024 7:25 AM
>>To: gdb-patches@sourceware.org
>>Cc: pedro@palves.net
>>Subject: [PING] [PATCH v2 0/6] fix PR gdb/19340
>>
>>Kindly pinging.
>>
>>Markus.
>>
>>>-----Original Message-----
>>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>>Sent: Thursday, April 11, 2024 7:26 AM
>>>To: gdb-patches@sourceware.org
>>>Cc: pedro@palves.net
>>>Subject: [PATCH v2 0/6] fix PR gdb/19340
>>>
>>>Fix PR gdb/19340 by stopping replaying the current thread before disabling
>>>recording.
>>>
>>>While writing the test for this, I wondered whether it would also work if
>>>there was more than one process and if some were recording while others
>>>were replaying.
>>>
>>>It turned out that recording one process while some other process is
>>>replaying doesn't work.  You first have to stop replaying everything
>>>before you can continue recording.  Since btrace is per inferior, this
>>>seems like an unnecessary restriction.
>>>
>>>The first patch fixes gdb/pr19340; the rest makes recording work together
>>>with replaying on inferior level.
>>>
>>>The most prominent change is probably that do_target_wait() no longer
>>>silently switches inferiors.  This infrun change requires global
>>>maintainer approval.
>>>
>>>Changes to v1:
>>>  - fixed a fail in a newly added test
>>>  - fixed fails in patch 5, which also caused the patch to get renamed
>>>
>>>Markus Metzger (6):
>>>  gdb, btrace: fix pr19340
>>>  gdb, btrace: simplify gdb.btrace/multi-inferior.exp
>>>  gdb, btrace: remove record_btrace_target::supports_*()
>>>  gdb, btrace: set wait status to ignore if nothing is moving
>>>  gdb, infrun: fix silent inferior switch in do_target_wait()
>>>  gdb, btrace, infrun: per-inferior run-control
>>>
>>> gdb/infrun.c                                | 63 ++++++++++++++---
>>> gdb/linux-nat.c                             | 17 +++--
>>> gdb/record-btrace.c                         | 78 +++++++--------------
>>> gdb/remote.c                                | 22 ++++--
>>> gdb/testsuite/gdb.btrace/multi-inferior.c   | 10 ++-
>>> gdb/testsuite/gdb.btrace/multi-inferior.exp | 33 ++++-----
>>> gdb/testsuite/gdb.btrace/step.exp           | 22 ++++--
>>> 7 files changed, 150 insertions(+), 95 deletions(-)
>>>
>>>--
>>>2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PING v4] [PATCH v2 0/6] fix PR gdb/19340

[Metzger, Markus T]

Kindly pinging.

I rebased and pushed the updated version to users/mmetzger/pr19340.
I also adjusted the tests to match changes to error messages.

Regards,
Markus.

>-----Original Message-----
>From: Metzger, Markus T <markus.t.metzger@intel.com>
>Sent: Tuesday, July 2, 2024 11:22 AM
>To: gdb-patches@sourceware.org
>Subject: [PING v3] [PATCH v2 0/6] fix PR gdb/19340
>
>Kindly pinging.
>
>Markus.
>
>>-----Original Message-----
>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>Sent: Friday, June 7, 2024 7:07 AM
>>To: gdb-patches@sourceware.org
>>Subject: [PING v2] [PATCH v2 0/6] fix PR gdb/19340
>>
>>Kindly pinging.
>>
>>Markus.
>>
>>>-----Original Message-----
>>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>>Sent: Friday, May 3, 2024 7:25 AM
>>>To: gdb-patches@sourceware.org
>>>Cc: pedro@palves.net
>>>Subject: [PING] [PATCH v2 0/6] fix PR gdb/19340
>>>
>>>Kindly pinging.
>>>
>>>Markus.
>>>
>>>>-----Original Message-----
>>>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>>>Sent: Thursday, April 11, 2024 7:26 AM
>>>>To: gdb-patches@sourceware.org
>>>>Cc: pedro@palves.net
>>>>Subject: [PATCH v2 0/6] fix PR gdb/19340
>>>>
>>>>Fix PR gdb/19340 by stopping replaying the current thread before disabling
>>>>recording.
>>>>
>>>>While writing the test for this, I wondered whether it would also work if
>>>>there was more than one process and if some were recording while others
>>>>were replaying.
>>>>
>>>>It turned out that recording one process while some other process is
>>>>replaying doesn't work.  You first have to stop replaying everything
>>>>before you can continue recording.  Since btrace is per inferior, this
>>>>seems like an unnecessary restriction.
>>>>
>>>>The first patch fixes gdb/pr19340; the rest makes recording work together
>>>>with replaying on inferior level.
>>>>
>>>>The most prominent change is probably that do_target_wait() no longer
>>>>silently switches inferiors.  This infrun change requires global
>>>>maintainer approval.
>>>>
>>>>Changes to v1:
>>>>  - fixed a fail in a newly added test
>>>>  - fixed fails in patch 5, which also caused the patch to get renamed
>>>>
>>>>Markus Metzger (6):
>>>>  gdb, btrace: fix pr19340
>>>>  gdb, btrace: simplify gdb.btrace/multi-inferior.exp
>>>>  gdb, btrace: remove record_btrace_target::supports_*()
>>>>  gdb, btrace: set wait status to ignore if nothing is moving
>>>>  gdb, infrun: fix silent inferior switch in do_target_wait()
>>>>  gdb, btrace, infrun: per-inferior run-control
>>>>
>>>> gdb/infrun.c                                | 63 ++++++++++++++---
>>>> gdb/linux-nat.c                             | 17 +++--
>>>> gdb/record-btrace.c                         | 78 +++++++--------------
>>>> gdb/remote.c                                | 22 ++++--
>>>> gdb/testsuite/gdb.btrace/multi-inferior.c   | 10 ++-
>>>> gdb/testsuite/gdb.btrace/multi-inferior.exp | 33 ++++-----
>>>> gdb/testsuite/gdb.btrace/step.exp           | 22 ++++--
>>>> 7 files changed, 150 insertions(+), 95 deletions(-)
>>>>
>>>>--
>>>>2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PING v5] [PATCH v2 0/6] fix PR gdb/19340

[Metzger, Markus T]

Kindly pinging.

Markus.

>-----Original Message-----
>From: Metzger, Markus T <markus.t.metzger@intel.com>
>Sent: Friday, September 13, 2024 12:43 PM
>To: gdb-patches@sourceware.org
>Subject: [PING v4] [PATCH v2 0/6] fix PR gdb/19340
>
>Kindly pinging.
>
>I rebased and pushed the updated version to users/mmetzger/pr19340.
>I also adjusted the tests to match changes to error messages.
>
>Regards,
>Markus.
>
>>-----Original Message-----
>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>Sent: Tuesday, July 2, 2024 11:22 AM
>>To: gdb-patches@sourceware.org
>>Subject: [PING v3] [PATCH v2 0/6] fix PR gdb/19340
>>
>>Kindly pinging.
>>
>>Markus.
>>
>>>-----Original Message-----
>>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>>Sent: Friday, June 7, 2024 7:07 AM
>>>To: gdb-patches@sourceware.org
>>>Subject: [PING v2] [PATCH v2 0/6] fix PR gdb/19340
>>>
>>>Kindly pinging.
>>>
>>>Markus.
>>>
>>>>-----Original Message-----
>>>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>>>Sent: Friday, May 3, 2024 7:25 AM
>>>>To: gdb-patches@sourceware.org
>>>>Cc: pedro@palves.net
>>>>Subject: [PING] [PATCH v2 0/6] fix PR gdb/19340
>>>>
>>>>Kindly pinging.
>>>>
>>>>Markus.
>>>>
>>>>>-----Original Message-----
>>>>>From: Metzger, Markus T <markus.t.metzger@intel.com>
>>>>>Sent: Thursday, April 11, 2024 7:26 AM
>>>>>To: gdb-patches@sourceware.org
>>>>>Cc: pedro@palves.net
>>>>>Subject: [PATCH v2 0/6] fix PR gdb/19340
>>>>>
>>>>>Fix PR gdb/19340 by stopping replaying the current thread before
>disabling
>>>>>recording.
>>>>>
>>>>>While writing the test for this, I wondered whether it would also work if
>>>>>there was more than one process and if some were recording while
>others
>>>>>were replaying.
>>>>>
>>>>>It turned out that recording one process while some other process is
>>>>>replaying doesn't work.  You first have to stop replaying everything
>>>>>before you can continue recording.  Since btrace is per inferior, this
>>>>>seems like an unnecessary restriction.
>>>>>
>>>>>The first patch fixes gdb/pr19340; the rest makes recording work together
>>>>>with replaying on inferior level.
>>>>>
>>>>>The most prominent change is probably that do_target_wait() no longer
>>>>>silently switches inferiors.  This infrun change requires global
>>>>>maintainer approval.
>>>>>
>>>>>Changes to v1:
>>>>>  - fixed a fail in a newly added test
>>>>>  - fixed fails in patch 5, which also caused the patch to get renamed
>>>>>
>>>>>Markus Metzger (6):
>>>>>  gdb, btrace: fix pr19340
>>>>>  gdb, btrace: simplify gdb.btrace/multi-inferior.exp
>>>>>  gdb, btrace: remove record_btrace_target::supports_*()
>>>>>  gdb, btrace: set wait status to ignore if nothing is moving
>>>>>  gdb, infrun: fix silent inferior switch in do_target_wait()
>>>>>  gdb, btrace, infrun: per-inferior run-control
>>>>>
>>>>> gdb/infrun.c                                | 63 ++++++++++++++---
>>>>> gdb/linux-nat.c                             | 17 +++--
>>>>> gdb/record-btrace.c                         | 78 +++++++--------------
>>>>> gdb/remote.c                                | 22 ++++--
>>>>> gdb/testsuite/gdb.btrace/multi-inferior.c   | 10 ++-
>>>>> gdb/testsuite/gdb.btrace/multi-inferior.exp | 33 ++++-----
>>>>> gdb/testsuite/gdb.btrace/step.exp           | 22 ++++--
>>>>> 7 files changed, 150 insertions(+), 95 deletions(-)
>>>>>
>>>>>--
>>>>>2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 5/6] gdb, infrun: fix silent inferior switch in do_target_wait()

[Andrew Burgess]

Markus Metzger <markus.t.metzger@intel.com> writes:

> In do_target_wait(), we iterate over inferiors and call
> do_target_wait_1(), which eventually calls target_wait() per inferior.
> Each time, we wait for minus_one_ptid.

Could you expand on this a little more please as I don't understand what
you are telling me, and I can't align this explanation with the code.

do_target_wait takes 'ptid_t wait_ptid' as an argument and never
mentions minus_one_ptid.  The wait_ptid is passed through to
do_target_wait_1.


do_target_wait_1 takes 'ptid_t ptid' as an argument (the value of which
comes from wait_ptid in do_target_wait), and this is passed through to
target_wait.  Further within do_target_wait_1 we have this:

  if (ptid == minus_one_ptid || ptid.is_pid ())

which would seem to indicate that we don't expect ptid to always be
minus_one_ptid.

So it's not clear to me what you mean when you say 'Each time, we wait
for minus_one_ptid'.  Maybe you mean elsewhere in GDB?

>
> In some cases, e.g. gdb.threads/detach-step-over.exp, we ask to wait for
> one inferior, and get an event from a different inferior back without
> noticing the inferior switch.

The way you mention detach-step-over.exp it seems like you think GDB is
doing something wrong.  But the test fully passes for me without this
patch, and you don't adjust the expected output.  So I'm guessing that
nothing changes for that test.  But that seems at odds with the say you
mention the test; my reading of the above is that GDB is doing something
wrong.  Could you clarify what's going on please.

>
> Wait for a single inferior, instead.  Since we iterate over all inferiors,
> we still cover everything.
>
> This exposes another bug with STOP_QUIETLY_NO_SIGSTOP handling.
>
> After attaching, we interrupt all threads in the new inferior, then call
> do_target_wait() to receive the stopped events.  This randomly selects an
> inferior to start waiting for and iterates over all inferiors starting
> from there.
>
> The initial stop event for the main thread is already queued up, so we
> wouldn't actually wait() if we had started with the new inferior.  Or if
> we had waited for minus_one_ptid, which would then have silently switched
> inferiors.
>
> Since we no longer allow that, we may actually wait() for the new inferior
> and find other events to report, out of which we randomly select one.
>
> If we selected an event for another thread, e.g. one that had been
> interrupted as part of non-stop attach, STOP_QUIETLY_NO_SIGSTOP would be
> applied to that thread (unnecessarily), leaving the main thread with a
> SIGSTOP event but last_resume_kind = 0 (resume_continue).

Just say 'resume_continue' here rather than '0'.  The last_resume_kind
field is an enum, not an integer.

>
> When the main thread is later selected, SIGSTOP is reported to the user.
>
> Normally, linux-nat's wait() turns the SIGSTOP it uses for interrupting
> threads into GDB_SIGNAL_0.  This is based on last_resume_kind, which is
> set to 2 (resume_stop) when sending SIGSTOP to interrupt a thread.

Same here, just say 'resume_stop', not '2'.

> We do this for all threads of the new inferior when interrupting them as
> part of non-stop attach.  Except for the main thread, which we expect to
> be reported before the first wait().
>
> Set last_resume_kind to resume_stop for the main thread after attaching.

I see that without this patch you do get some failures in the next patch
of this series, and my next job is to debug those failures in order to
understand this patch more...

... but is there any chance we could write a test that exposes an issue
that is fixed by this patch, that could be added just with this patch?

> ---
>  gdb/infrun.c    | 46 +++++++++++++++++++++++++++++++++++++++++++---
>  gdb/linux-nat.c | 17 ++++++++++++-----
>  gdb/remote.c    | 22 +++++++++++++++++-----
>  3 files changed, 72 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index a5030b16376..9ca0571065c 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -4198,7 +4198,23 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
>  
>    auto do_wait = [&] (inferior *inf)
>    {
> -    ecs->ptid = do_target_wait_1 (inf, wait_ptid, &ecs->ws, options);
> +    ptid_t ptid { inf->pid };
> +
> +    /* Make sure we're not widening WAIT_PTID.  */
> +    if (!ptid.matches (wait_ptid)

Sorry, I just don't understand what's going on here.

If wait_ptid is minus_one_ptid then ptid will be for pid -1, and the
ptid.matches() call will return true.  But a ptid_t initialised with
only a pid of -1 is equal to minus_one_ptid, right?

If wait_ptid is for a process only, with no thread, then ptid ==
wait_ptid, and ptid.matches() returns true.

If wait_ptid is for a thread then ptid.matches() returns false, and we
revert to using wait_ptid.

So, if my understanding is correct, ptid always ends up equal to
wait_ptid.


> +	/* Targets that cannot async will be asked for a blocking wait.
> +
> +	   Blocking wait does not work inferior-by-inferior if the target
> +	   provides more than one inferior.  Fall back to waiting for
> +	   WAIT_PTID in that case.  */
> +	|| !target_can_async_p () || ((options & TARGET_WNOHANG) == 0)
> +	/* FIXME: I don't see why we should have inferiors with zero pid,
> +	   which indicates that the respective ptid is not a process.
> +	   They do exist, though, and we cannot wait for them.  */

I'm not a fan of FIXME comments that don't lay out a clear path for how
to fix the issue.  e.g. "FIXME: This can't be changed until GCC 33.1 is
released with support for DWARF8" or some such.

I cannot offer much insight here, but you do say "They do exist, though,
..." so you've clearly tracked down where they come from.  So why not
share those details.  Better yet, rewrite the comment as an explanation:

  "Process with a zero pid are created by ...... Unfortunately we cannot
   wait for a zero pid so revert to using the full wait_ptid."

> +	|| !ptid.is_pid ())
> +      ptid = wait_ptid;
> +
> +    ecs->ptid = do_target_wait_1 (inf, ptid, &ecs->ws, options);
>      ecs->target = inf->process_target ();
>      return (ecs->ws.kind () != TARGET_WAITKIND_IGNORE);
>    };
> @@ -4208,6 +4224,12 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
>       reported the stop to the user, polling for events.  */
>    scoped_restore_current_thread restore_thread;
>  
> +  /* The first TARGET_WAITKIND_NO_RESUMED execution state.
> +
> +     If we do not find a more interesting event, we will report that.  */

I think this comment needs rewording.  I had to study the code before
this made sense to me.  By default the no_resumed object's `ws` member
will be TARGET_WAITKIND_IGNORE, so initially I didn't understand what
the TARGET_WAITKIND_NO_RESUMED comment was going on about.

I think the comment should explain how the object will be copied into
later on when we find a TARGET_WAITKIND_NO_RESUMED result.

I'm continuing to review this patch, but answers to some of the above
(and a test?) would help a lot.

Thanks,
Andrew


> +  execution_control_state no_resumed {};
> +  no_resumed.ptid = null_ptid;
> +
>    intrusive_list_iterator<inferior> start
>      = inferior_list.iterator_to (*selected);
>  
> @@ -4218,7 +4240,13 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
>        inferior *inf = &*it;
>  
>        if (inferior_matches (inf) && do_wait (inf))
> -	return true;
> +	{
> +	  if (ecs->ws.kind () != TARGET_WAITKIND_NO_RESUMED)
> +	    return true;
> +
> +	  if (no_resumed.ptid == null_ptid)
> +	    no_resumed = *ecs;
> +	}
>      }
>  
>    for (intrusive_list_iterator<inferior> it = inferior_list.begin ();
> @@ -4228,7 +4256,19 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
>        inferior *inf = &*it;
>  
>        if (inferior_matches (inf) && do_wait (inf))
> -	return true;
> +	{
> +	  if (ecs->ws.kind () != TARGET_WAITKIND_NO_RESUMED)
> +	    return true;
> +
> +	  if (no_resumed.ptid == null_ptid)
> +	    no_resumed = *ecs;
> +	}
> +    }
> +
> +  if (no_resumed.ptid != null_ptid)
> +    {
> +      *ecs = no_resumed;
> +      return true;
>      }
>  
>    ecs->ws.set_ignore ();
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 2602e1f240d..06b39d67a72 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1154,6 +1154,7 @@ linux_nat_target::attach (const char *args, int from_tty)
>  
>    /* Add the initial process as the first LWP to the list.  */
>    lp = add_initial_lwp (ptid);
> +  lp->last_resume_kind = resume_stop;
>  
>    status = linux_nat_post_attach_wait (lp->ptid, &lp->signalled);
>    if (!WIFSTOPPED (status))
> @@ -3329,12 +3330,18 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
>       moment at which we know its PID.  */
>    if (ptid.is_pid () && find_lwp_pid (ptid) == nullptr)
>      {
> -      ptid_t lwp_ptid (ptid.pid (), ptid.pid ());
> +      /* Unless we already did and this is simply a request to wait for a
> +	 particular inferior.  */
> +      inferior *inf = find_inferior_ptid (linux_target, ptid);
> +      if (inf && inf->find_thread (ptid))
> +	{
> +	  ptid_t lwp_ptid (ptid.pid (), ptid.pid ());
>  
> -      /* Upgrade the main thread's ptid.  */
> -      thread_change_ptid (linux_target, ptid, lwp_ptid);
> -      lp = add_initial_lwp (lwp_ptid);
> -      lp->resumed = 1;
> +	  /* Upgrade the main thread's ptid.  */
> +	  thread_change_ptid (linux_target, ptid, lwp_ptid);
> +	  lp = add_initial_lwp (lwp_ptid);
> +	  lp->resumed = 1;
> +	}
>      }
>  
>    /* Make sure SIGCHLD is blocked until the sigsuspend below.  */
> diff --git a/gdb/remote.c b/gdb/remote.c
> index a09ba4d715d..49abd4e4376 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -7825,12 +7825,24 @@ remote_target::remote_notif_remove_queued_reply (ptid_t ptid)
>  {
>    remote_state *rs = get_remote_state ();
>  
> +  auto pred = [=] (const stop_reply_up &event)
> +  {
> +    /* A null ptid should only happen if we have a single process.  It
> +       wouldn't match the process ptid, though, so let's check this case
> +       separately.  */
> +    if ((event->ptid == null_ptid) && ptid.is_pid ())
> +      return true;
> +
> +    /* A minus one ptid should only happen for events that match
> +       everything.  It wouldn't match a process or thread ptid, though, so
> +       let's check this case separately.  */
> +    if (event->ptid == minus_one_ptid)
> +      return true;
> +
> +    return event->ptid.matches (ptid);
> +  };
>    auto iter = std::find_if (rs->stop_reply_queue.begin (),
> -			    rs->stop_reply_queue.end (),
> -			    [=] (const stop_reply_up &event)
> -			    {
> -			      return event->ptid.matches (ptid);
> -			    });
> +			    rs->stop_reply_queue.end (), pred);
>    stop_reply_up result;
>    if (iter != rs->stop_reply_queue.end ())
>      {
> -- 
> 2.34.1
>
> 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 v2 5/6] gdb, infrun: fix silent inferior switch in do_target_wait()

[Metzger, Markus T]

Hello Andrew,

>> In do_target_wait(), we iterate over inferiors and call
>> do_target_wait_1(), which eventually calls target_wait() per inferior.
>> Each time, we wait for minus_one_ptid.
>
>Could you expand on this a little more please as I don't understand what
>you are telling me, and I can't align this explanation with the code.

This used to be minus_one_ptid until you changed it (back) to wait_ptid in
07505b613a0605ef42a004364ab8ccb70fd3c8eb.

There is a single call of that function that passes minus_one_ptid outside
of condition evaluation:

    /* If the thread is in the middle of the condition evaluation, wait for
       an event from the current thread.  Otherwise, wait for an event from
       any thread.  */
    ptid_t waiton_ptid = in_cond_eval ? inferior_ptid : minus_one_ptid;

    if (!do_target_wait (waiton_ptid, &ecs, TARGET_WNOHANG))
      {
	infrun_debug_printf ("do_target_wait returned no event");
	disable_commit_resumed.reset_and_commit ();
	return;
      }

>So it's not clear to me what you mean when you say 'Each time, we wait
>for minus_one_ptid'.  Maybe you mean elsewhere in GDB?

I should say 'outside of condition evaluation, we wait for minus_one_ptid'.


>> In some cases, e.g. gdb.threads/detach-step-over.exp, we ask to wait for
>> one inferior, and get an event from a different inferior back without
>> noticing the inferior switch.
>
>The way you mention detach-step-over.exp it seems like you think GDB is
>doing something wrong.  But the test fully passes for me without this
>patch, and you don't adjust the expected output.  So I'm guessing that
>nothing changes for that test.  But that seems at odds with the say you
>mention the test; my reading of the above is that GDB is doing something
>wrong.  Could you clarify what's going on please.

It is definitely wrong to go through all the effort of randomly selecting an inferior
and then iterating over inferiors one-by-one if we then allow switching to a different
inferior in the actual wait call.


>I see that without this patch you do get some failures in the next patch
>of this series, and my next job is to debug those failures in order to
>understand this patch more...

The next patch organizes the replay/record state per inferior.  A single target
may have inferiors that are currently recording and inferiors that are currently
replaying.  We need to wait for individual inferiors since they are handled by
different target methods: the replaying ones are handled by record targets and
the recording ones are handled by the target beneath the record target.

We could handle this in the record target by splitting forwarding the request
for some threads and handling it itself for others.

Since the infrun do_target_wait() code obviously intends to do the wait per
inferior and just forgets to adjust the wait focus to one inferior, fixing that
seemed to be the better approach to me.


>... but is there any chance we could write a test that exposes an issue
>that is fixed by this patch, that could be added just with this patch?
>
>> ---
>>  gdb/infrun.c    | 46 +++++++++++++++++++++++++++++++++++++++++++---
>>  gdb/linux-nat.c | 17 ++++++++++++-----
>>  gdb/remote.c    | 22 +++++++++++++++++-----
>>  3 files changed, 72 insertions(+), 13 deletions(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index a5030b16376..9ca0571065c 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -4198,7 +4198,23 @@ do_target_wait (ptid_t wait_ptid,
>execution_control_state *ecs,
>>
>>    auto do_wait = [&] (inferior *inf)
>>    {
>> -    ecs->ptid = do_target_wait_1 (inf, wait_ptid, &ecs->ws, options);
>> +    ptid_t ptid { inf->pid };
>> +
>> +    /* Make sure we're not widening WAIT_PTID.  */
>> +    if (!ptid.matches (wait_ptid)
>
>Sorry, I just don't understand what's going on here.
>
>If wait_ptid is minus_one_ptid then ptid will be for pid -1, and the
>ptid.matches() call will return true.  But a ptid_t initialised with
>only a pid of -1 is equal to minus_one_ptid, right?

Right, but this ptid is initialized to inf->pid, not to wait_ptid.pid ().


>If wait_ptid is for a process only, with no thread, then ptid ==
>wait_ptid, and ptid.matches() returns true.
>
>If wait_ptid is for a thread then ptid.matches() returns false, and we
>revert to using wait_ptid.
>
>So, if my understanding is correct, ptid always ends up equal to
>wait_ptid.
>
>
>> +	/* Targets that cannot async will be asked for a blocking wait.
>> +
>> +	   Blocking wait does not work inferior-by-inferior if the target
>> +	   provides more than one inferior.  Fall back to waiting for
>> +	   WAIT_PTID in that case.  */
>> +	|| !target_can_async_p () || ((options & TARGET_WNOHANG) == 0)
>> +	/* FIXME: I don't see why we should have inferiors with zero pid,
>> +	   which indicates that the respective ptid is not a process.
>> +	   They do exist, though, and we cannot wait for them.  */
>
>I'm not a fan of FIXME comments that don't lay out a clear path for how
>to fix the issue.  e.g. "FIXME: This can't be changed until GCC 33.1 is
>released with support for DWARF8" or some such.
>
>I cannot offer much insight here, but you do say "They do exist, though,
>..." so you've clearly tracked down where they come from.  So why not
>share those details.  Better yet, rewrite the comment as an explanation:
>
>  "Process with a zero pid are created by ...... Unfortunately we cannot
>   wait for a zero pid so revert to using the full wait_ptid."

One such inferior is created by initialize_inferiors ():
  set_current_inferior (add_inferior_silent (0));

It is strange that the ptid for such an inferior says that this isn't a process.

I changed the comment to:
	/* We cannot wait for inferiors without a pid.

	   One such inferior is created by initialize_inferiors () to
	   ensure that there always is an inferior.  */


>> +	|| !ptid.is_pid ())
>> +      ptid = wait_ptid;
>> +
>> +    ecs->ptid = do_target_wait_1 (inf, ptid, &ecs->ws, options);
>>      ecs->target = inf->process_target ();
>>      return (ecs->ws.kind () != TARGET_WAITKIND_IGNORE);
>>    };
>> @@ -4208,6 +4224,12 @@ do_target_wait (ptid_t wait_ptid,
>execution_control_state *ecs,
>>       reported the stop to the user, polling for events.  */
>>    scoped_restore_current_thread restore_thread;
>>
>> +  /* The first TARGET_WAITKIND_NO_RESUMED execution state.
>> +
>> +     If we do not find a more interesting event, we will report that.  */
>
>I think this comment needs rewording.  I had to study the code before
>this made sense to me.  By default the no_resumed object's `ws` member
>will be TARGET_WAITKIND_IGNORE, so initially I didn't understand what
>the TARGET_WAITKIND_NO_RESUMED comment was going on about.
>
>I think the comment should explain how the object will be copied into
>later on when we find a TARGET_WAITKIND_NO_RESUMED result.

Yeah, this is another side-effect of not adjusting the wait scope.  If you're
waiting for minus_one_ptid, the target will check all its inferiors and only
return TARGET_WAITKIND_NO_RESUMED if no other thread has anything
to report.

It's still wrong in the case of multi-target, where we could have an event on
some other target, yet we still return the TARGET_WAITKIND_NO_RESUMED
from the first target we checked.

I changed the comment to:
  /* The first TARGET_WAITKIND_NO_RESUMED execution state.

     We do not want to return TARGET_WAITKIND_NO_RESUMED right away since
     another inferior may have a more interesting event to report.  If
     there is no other event to report, after all, we want to report the
     first such event.

     This variable holds that first event, which will be copied on the
     first TARGET_WAITKIND_NO_RESUMED below.  */


>I'm continuing to review this patch, but answers to some of the above
>(and a test?) would help a lot.

Here come answers to the above.  I don't have a test that exploits this
inferior switching.  I noticed it when I debugged gdb.threads/detach-step-over.exp.
I don't remember why exactly I debugged it.  There were fails, of course, but
they were not related to the inferior switching.

Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH v2 5/6] gdb, infrun: fix silent inferior switch in do_target_wait()

[Andrew Burgess]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> Hello Andrew,
>
>>> In do_target_wait(), we iterate over inferiors and call
>>> do_target_wait_1(), which eventually calls target_wait() per inferior.
>>> Each time, we wait for minus_one_ptid.
>>
>>Could you expand on this a little more please as I don't understand what
>>you are telling me, and I can't align this explanation with the code.
>
> This used to be minus_one_ptid until you changed it (back) to wait_ptid in
> 07505b613a0605ef42a004364ab8ccb70fd3c8eb.
>
> There is a single call of that function that passes minus_one_ptid outside
> of condition evaluation:
>
>     /* If the thread is in the middle of the condition evaluation, wait for
>        an event from the current thread.  Otherwise, wait for an event from
>        any thread.  */
>     ptid_t waiton_ptid = in_cond_eval ? inferior_ptid : minus_one_ptid;
>
>     if (!do_target_wait (waiton_ptid, &ecs, TARGET_WNOHANG))
>       {
> 	infrun_debug_printf ("do_target_wait returned no event");
> 	disable_commit_resumed.reset_and_commit ();
> 	return;
>       }
>
>>So it's not clear to me what you mean when you say 'Each time, we wait
>>for minus_one_ptid'.  Maybe you mean elsewhere in GDB?
>
> I should say 'outside of condition evaluation, we wait for minus_one_ptid'.
>
>
>>> In some cases, e.g. gdb.threads/detach-step-over.exp, we ask to wait for
>>> one inferior, and get an event from a different inferior back without
>>> noticing the inferior switch.
>>
>>The way you mention detach-step-over.exp it seems like you think GDB is
>>doing something wrong.  But the test fully passes for me without this
>>patch, and you don't adjust the expected output.  So I'm guessing that
>>nothing changes for that test.  But that seems at odds with the say you
>>mention the test; my reading of the above is that GDB is doing something
>>wrong.  Could you clarify what's going on please.
>
> It is definitely wrong to go through all the effort of randomly selecting an inferior
> and then iterating over inferiors one-by-one if we then allow switching to a different
> inferior in the actual wait call.
>
>
>>I see that without this patch you do get some failures in the next patch
>>of this series, and my next job is to debug those failures in order to
>>understand this patch more...
>
> The next patch organizes the replay/record state per inferior.  A single target
> may have inferiors that are currently recording and inferiors that are currently
> replaying.  We need to wait for individual inferiors since they are handled by
> different target methods: the replaying ones are handled by record targets and
> the recording ones are handled by the target beneath the record target.
>
> We could handle this in the record target by splitting forwarding the request
> for some threads and handling it itself for others.
>
> Since the infrun do_target_wait() code obviously intends to do the wait per
> inferior and just forgets to adjust the wait focus to one inferior, fixing that
> seemed to be the better approach to me.

I agree with you.  But I do have one concern.  As part of the next
commit I think you should add:

  gdb_assert (ptid != minus_one_ptid);

to the start of `record_btrace_target::wait`.  For the reasons you
explain, this function can't currently wait on all inferiors, if one
inferior requires handling in record_btrace_target while another
inferior requires handling in the target beneath.

Which leads to my concern.  With that assert in place we are, I claim,
making the promise that target_ops::wait() will _never_ be called with
minus_one_ptid, but this code:

    /* Make sure we're not widening WAIT_PTID.  */
    if (!ptid.matches (wait_ptid)
	/* Targets that cannot async will be asked for a blocking wait.

	   Blocking wait does not work inferior-by-inferior if the target
	   provides more than one inferior.  Fall back to waiting for
	   WAIT_PTID in that case.  */
	|| !target_can_async_p () || ((options & TARGET_WNOHANG) == 0)
	/* FIXME: I don't see why we should have inferiors with zero pid,
	   which indicates that the respective ptid is not a process.
	   They do exist, though, and we cannot wait for them.  */
	|| !ptid.is_pid ())
      ptid = wait_ptid;

From your branch, in infrun.c, would, if I understand it correctly, mean
that ptid could be set to minus_one_ptid in some cases.

So my concern here is: are there situations where
record_btrace_target::wait can end up being called with minus_one_ptid?

Lets be clear, I totally agree with you that for the case where
TARGET_WNOHANG is set, it makes more sense that we wait on each inferior
in turn.  But I wonder if we should still be changing
record_btrace_target::wait so that it can handle minus_one_ptid?

I applied this HACK:

  diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
  index 7937bb3f8db..367225c3320 100644
  --- a/gdb/record-btrace.c
  +++ b/gdb/record-btrace.c
  @@ -2637,6 +2637,13 @@ record_btrace_target::wait (ptid_t ptid, struct target_waitstatus *status,
   
     if (moving.empty ())
       {
  +      if ((::execution_direction != EXEC_REVERSE)
  +	  && ptid == minus_one_ptid)
  +	{
  +	  infrun_debug_printf ("forwarding the wait to target beneath");
  +	  return this->beneath ()->wait (ptid, status, options);
  +	}
  +
         *status = btrace_step_no_moving_threads ();
   
         DEBUG ("wait ended by %s: %s", null_ptid.to_string ().c_str (),

and revert patch 5/6 in your series, and all of your new tests still
pass.  I don't think the above is the correct way to change ::wait, but
it does hint that it might be possible to support minus_one_ptid.

As I said, I do agree with you that do_target_wait could be improved,
but I still need convincing that record_btrace_target::wait is OK as is.

Thanks,
Andrew



>
>
>>... but is there any chance we could write a test that exposes an issue
>>that is fixed by this patch, that could be added just with this patch?
>>
>>> ---
>>>  gdb/infrun.c    | 46 +++++++++++++++++++++++++++++++++++++++++++---
>>>  gdb/linux-nat.c | 17 ++++++++++++-----
>>>  gdb/remote.c    | 22 +++++++++++++++++-----
>>>  3 files changed, 72 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>>> index a5030b16376..9ca0571065c 100644
>>> --- a/gdb/infrun.c
>>> +++ b/gdb/infrun.c
>>> @@ -4198,7 +4198,23 @@ do_target_wait (ptid_t wait_ptid,
>>execution_control_state *ecs,
>>>
>>>    auto do_wait = [&] (inferior *inf)
>>>    {
>>> -    ecs->ptid = do_target_wait_1 (inf, wait_ptid, &ecs->ws, options);
>>> +    ptid_t ptid { inf->pid };
>>> +
>>> +    /* Make sure we're not widening WAIT_PTID.  */
>>> +    if (!ptid.matches (wait_ptid)
>>
>>Sorry, I just don't understand what's going on here.
>>
>>If wait_ptid is minus_one_ptid then ptid will be for pid -1, and the
>>ptid.matches() call will return true.  But a ptid_t initialised with
>>only a pid of -1 is equal to minus_one_ptid, right?
>
> Right, but this ptid is initialized to inf->pid, not to wait_ptid.pid ().
>
>
>>If wait_ptid is for a process only, with no thread, then ptid ==
>>wait_ptid, and ptid.matches() returns true.
>>
>>If wait_ptid is for a thread then ptid.matches() returns false, and we
>>revert to using wait_ptid.
>>
>>So, if my understanding is correct, ptid always ends up equal to
>>wait_ptid.
>>
>>
>>> +	/* Targets that cannot async will be asked for a blocking wait.
>>> +
>>> +	   Blocking wait does not work inferior-by-inferior if the target
>>> +	   provides more than one inferior.  Fall back to waiting for
>>> +	   WAIT_PTID in that case.  */
>>> +	|| !target_can_async_p () || ((options & TARGET_WNOHANG) == 0)
>>> +	/* FIXME: I don't see why we should have inferiors with zero pid,
>>> +	   which indicates that the respective ptid is not a process.
>>> +	   They do exist, though, and we cannot wait for them.  */
>>
>>I'm not a fan of FIXME comments that don't lay out a clear path for how
>>to fix the issue.  e.g. "FIXME: This can't be changed until GCC 33.1 is
>>released with support for DWARF8" or some such.
>>
>>I cannot offer much insight here, but you do say "They do exist, though,
>>..." so you've clearly tracked down where they come from.  So why not
>>share those details.  Better yet, rewrite the comment as an explanation:
>>
>>  "Process with a zero pid are created by ...... Unfortunately we cannot
>>   wait for a zero pid so revert to using the full wait_ptid."
>
> One such inferior is created by initialize_inferiors ():
>   set_current_inferior (add_inferior_silent (0));
>
> It is strange that the ptid for such an inferior says that this isn't a process.
>
> I changed the comment to:
> 	/* We cannot wait for inferiors without a pid.
>
> 	   One such inferior is created by initialize_inferiors () to
> 	   ensure that there always is an inferior.  */
>
>
>>> +	|| !ptid.is_pid ())
>>> +      ptid = wait_ptid;
>>> +
>>> +    ecs->ptid = do_target_wait_1 (inf, ptid, &ecs->ws, options);
>>>      ecs->target = inf->process_target ();
>>>      return (ecs->ws.kind () != TARGET_WAITKIND_IGNORE);
>>>    };
>>> @@ -4208,6 +4224,12 @@ do_target_wait (ptid_t wait_ptid,
>>execution_control_state *ecs,
>>>       reported the stop to the user, polling for events.  */
>>>    scoped_restore_current_thread restore_thread;
>>>
>>> +  /* The first TARGET_WAITKIND_NO_RESUMED execution state.
>>> +
>>> +     If we do not find a more interesting event, we will report that.  */
>>
>>I think this comment needs rewording.  I had to study the code before
>>this made sense to me.  By default the no_resumed object's `ws` member
>>will be TARGET_WAITKIND_IGNORE, so initially I didn't understand what
>>the TARGET_WAITKIND_NO_RESUMED comment was going on about.
>>
>>I think the comment should explain how the object will be copied into
>>later on when we find a TARGET_WAITKIND_NO_RESUMED result.
>
> Yeah, this is another side-effect of not adjusting the wait scope.  If you're
> waiting for minus_one_ptid, the target will check all its inferiors and only
> return TARGET_WAITKIND_NO_RESUMED if no other thread has anything
> to report.
>
> It's still wrong in the case of multi-target, where we could have an event on
> some other target, yet we still return the TARGET_WAITKIND_NO_RESUMED
> from the first target we checked.
>
> I changed the comment to:
>   /* The first TARGET_WAITKIND_NO_RESUMED execution state.
>
>      We do not want to return TARGET_WAITKIND_NO_RESUMED right away since
>      another inferior may have a more interesting event to report.  If
>      there is no other event to report, after all, we want to report the
>      first such event.
>
>      This variable holds that first event, which will be copied on the
>      first TARGET_WAITKIND_NO_RESUMED below.  */
>
>
>>I'm continuing to review this patch, but answers to some of the above
>>(and a test?) would help a lot.
>
> Here come answers to the above.  I don't have a test that exploits this
> inferior switching.  I noticed it when I debugged gdb.threads/detach-step-over.exp.
> I don't remember why exactly I debugged it.  There were fails, of course, but
> they were not related to the inferior switching.
>
> Thanks,
> Markus.
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH v2 5/6] gdb, infrun: fix silent inferior switch in do_target_wait()

[Metzger, Markus T]

Hello Andrew,

>> Since the infrun do_target_wait() code obviously intends to do the wait per
>> inferior and just forgets to adjust the wait focus to one inferior, fixing that
>> seemed to be the better approach to me.
>
>I agree with you.  But I do have one concern.  As part of the next
>commit I think you should add:
>
>  gdb_assert (ptid != minus_one_ptid);

To trigger an issue, you would need two inferiors on the target, one of
which is replaying and the other is recording.

That assert is too aggressive IMHO.

Also, the issue is not fatal.  You should be able to C-c and recover by
replaying both inferiors.  That's certainly not what we'd want, but that's
better than terminating the debug session with an assert.


>Which leads to my concern.  With that assert in place we are, I claim,
>making the promise that target_ops::wait() will _never_ be called with
>minus_one_ptid, but this code:
>
>    /* Make sure we're not widening WAIT_PTID.  */
>    if (!ptid.matches (wait_ptid)
>	/* Targets that cannot async will be asked for a blocking wait.
>
>	   Blocking wait does not work inferior-by-inferior if the target
>	   provides more than one inferior.  Fall back to waiting for
>	   WAIT_PTID in that case.  */
>	|| !target_can_async_p () || ((options & TARGET_WNOHANG) == 0)
>	/* FIXME: I don't see why we should have inferiors with zero pid,
>	   which indicates that the respective ptid is not a process.
>	   They do exist, though, and we cannot wait for them.  */
>	|| !ptid.is_pid ())
>      ptid = wait_ptid;
>
>From your branch, in infrun.c, would, if I understand it correctly, mean
>that ptid could be set to minus_one_ptid in some cases.
>
>So my concern here is: are there situations where
>record_btrace_target::wait can end up being called with minus_one_ptid?

Sure.

The above code snippet gives a recipe how to construct such a scenario.
E.g. 'maintenance set target-async off' should do the trick.

I tested this with native, native-gdbserver, and native-extended-gdbserver
and I have not seen fails in gdb.btrace tests, so I believe that with the in-tree
targets that support btrace it should work by default.  That does not mean
that there couldn't be an out-of-tree target that would fail.  Or that future
changes to GDB couldn't break this.


>Lets be clear, I totally agree with you that for the case where
>TARGET_WNOHANG is set, it makes more sense that we wait on each inferior
>in turn.  But I wonder if we should still be changing
>record_btrace_target::wait so that it can handle minus_one_ptid?

Actually, I don't see how the current do_target_wait() code could work
with multiple blocking targets.

It will prioritize a random target and then just hang, ignoring other
targets that might have events to report.  You don't even need multiple
blocking targets; it suffices if one target is blocking.

E.g. if you step one thread with scheduler-locking off (the default) and
then randomly pick a thread on a different target that happens to be
blocking.


>I applied this HACK:
>
>  diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
>  index 7937bb3f8db..367225c3320 100644
>  --- a/gdb/record-btrace.c
>  +++ b/gdb/record-btrace.c
>  @@ -2637,6 +2637,13 @@ record_btrace_target::wait (ptid_t ptid, struct
>target_waitstatus *status,
>
>     if (moving.empty ())
>       {
>  +      if ((::execution_direction != EXEC_REVERSE)
>  +	  && ptid == minus_one_ptid)
>  +	{
>  +	  infrun_debug_printf ("forwarding the wait to target beneath");
>  +	  return this->beneath ()->wait (ptid, status, options);
>  +	}
>  +
>         *status = btrace_step_no_moving_threads ();
>
>         DEBUG ("wait ended by %s: %s", null_ptid.to_string ().c_str (),
>
>and revert patch 5/6 in your series, and all of your new tests still
>pass.  I don't think the above is the correct way to change ::wait, but
>it does hint that it might be possible to support minus_one_ptid.
>
>As I said, I do agree with you that do_target_wait could be improved,
>but I still need convincing that record_btrace_target::wait is OK as is.

Without NOHANG or with a non-async target, the target beneath does
a blocking wait.  This would prevent us from interleaving replaying and
recording as we would normally when waiting inferior-by-inferior with
NOHANG on async targets.

We would very likely end up prioritizing replaying.  We cannot afford to
forward the request to a potentially blocking target if there is work on
the replaying side.

If there is no work, however, it should be safe to forward the request
and have the target beneath block or respond with NO_RESUMED.

Let me experiment with that.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH v2 5/6] gdb, infrun: fix silent inferior switch in do_target_wait()

[Andrew Burgess]

"Metzger, Markus T" <markus.t.metzger@intel.com> writes:

> Hello Andrew,
>
>>> Since the infrun do_target_wait() code obviously intends to do the wait per
>>> inferior and just forgets to adjust the wait focus to one inferior, fixing that
>>> seemed to be the better approach to me.
>>
>>I agree with you.  But I do have one concern.  As part of the next
>>commit I think you should add:
>>
>>  gdb_assert (ptid != minus_one_ptid);
>
> To trigger an issue, you would need two inferiors on the target, one of
> which is replaying and the other is recording.
>
> That assert is too aggressive IMHO.

I don't understand how you reach this conclusion, because...

>
> Also, the issue is not fatal.  You should be able to C-c and recover by
> replaying both inferiors.  That's certainly not what we'd want, but that's
> better than terminating the debug session with an assert.

... you seem to be saying that GDB can get into a non-responsive state,
but it's OK as a user can interrupt and fix it.  But that doesn't seem
great.  Wouldn't it be better to just fix record_btrace_target::wait so
that it handles minus_one_ptid correctly? 

>>Which leads to my concern.  With that assert in place we are, I claim,
>>making the promise that target_ops::wait() will _never_ be called with
>>minus_one_ptid, but this code:
>>
>>    /* Make sure we're not widening WAIT_PTID.  */
>>    if (!ptid.matches (wait_ptid)
>>	/* Targets that cannot async will be asked for a blocking wait.
>>
>>	   Blocking wait does not work inferior-by-inferior if the target
>>	   provides more than one inferior.  Fall back to waiting for
>>	   WAIT_PTID in that case.  */
>>	|| !target_can_async_p () || ((options & TARGET_WNOHANG) == 0)
>>	/* FIXME: I don't see why we should have inferiors with zero pid,
>>	   which indicates that the respective ptid is not a process.
>>	   They do exist, though, and we cannot wait for them.  */
>>	|| !ptid.is_pid ())
>>      ptid = wait_ptid;
>>
>>From your branch, in infrun.c, would, if I understand it correctly, mean
>>that ptid could be set to minus_one_ptid in some cases.
>>
>>So my concern here is: are there situations where
>>record_btrace_target::wait can end up being called with minus_one_ptid?
>
> Sure.
>
> The above code snippet gives a recipe how to construct such a scenario.
> E.g. 'maintenance set target-async off' should do the trick.
>
> I tested this with native, native-gdbserver, and native-extended-gdbserver
> and I have not seen fails in gdb.btrace tests, so I believe that with the in-tree
> targets that support btrace it should work by default.

I'm super confused, if we know that we can trigger
record_btrace_target::wait to be called with minus_one_ptid, and we know
there's a configuration in which record_btrace_target::wait is going to
fail to find an event, then it's not really about whether any existing
test fails or not, it's about whether GDB could enter that state or not.

You said the minus_one_ptid assert is too aggressive, but the whole
premise of this patch is that in a multi-inferior setup, if we call
record_btrace_target::wait with minus_one_ptid then GDB can miss an
event, right?  So to me we have two choices, guarantee that
record_btrace_target::wait is never call with minus_one_ptid, or make
record_btrace_target::wait handle minus_one_ptid correctly.

>                                                         That does not mean
> that there couldn't be an out-of-tree target that would fail.  Or that future
> changes to GDB couldn't break this.

I'm not fussed about out of tree stuff, but it sounds like there are
problems with the current in-tree stuff...

>
>
>>Lets be clear, I totally agree with you that for the case where
>>TARGET_WNOHANG is set, it makes more sense that we wait on each inferior
>>in turn.  But I wonder if we should still be changing
>>record_btrace_target::wait so that it can handle minus_one_ptid?
>
> Actually, I don't see how the current do_target_wait() code could work
> with multiple blocking targets.
>
> It will prioritize a random target and then just hang, ignoring other
> targets that might have events to report.  You don't even need multiple
> blocking targets; it suffices if one target is blocking.
>
> E.g. if you step one thread with scheduler-locking off (the default) and
> then randomly pick a thread on a different target that happens to be
> blocking.

Yeah, I don't think the multi-inferior stuff will work with blocking
targets.

>
>
>>I applied this HACK:
>>
>>  diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
>>  index 7937bb3f8db..367225c3320 100644
>>  --- a/gdb/record-btrace.c
>>  +++ b/gdb/record-btrace.c
>>  @@ -2637,6 +2637,13 @@ record_btrace_target::wait (ptid_t ptid, struct
>>target_waitstatus *status,
>>
>>     if (moving.empty ())
>>       {
>>  +      if ((::execution_direction != EXEC_REVERSE)
>>  +	  && ptid == minus_one_ptid)
>>  +	{
>>  +	  infrun_debug_printf ("forwarding the wait to target beneath");
>>  +	  return this->beneath ()->wait (ptid, status, options);
>>  +	}
>>  +
>>         *status = btrace_step_no_moving_threads ();
>>
>>         DEBUG ("wait ended by %s: %s", null_ptid.to_string ().c_str (),
>>
>>and revert patch 5/6 in your series, and all of your new tests still
>>pass.  I don't think the above is the correct way to change ::wait, but
>>it does hint that it might be possible to support minus_one_ptid.
>>
>>As I said, I do agree with you that do_target_wait could be improved,
>>but I still need convincing that record_btrace_target::wait is OK as is.
>
> Without NOHANG or with a non-async target, the target beneath does
> a blocking wait.  This would prevent us from interleaving replaying and
> recording as we would normally when waiting inferior-by-inferior with
> NOHANG on async targets.
>
> We would very likely end up prioritizing replaying.  We cannot afford to
> forward the request to a potentially blocking target if there is work on
> the replaying side.
>
> If there is no work, however, it should be safe to forward the request
> and have the target beneath block or respond with NO_RESUMED.
>
> Let me experiment with that.

OK, will look out for an update.

Thanks,
Andrew