[3/4] gdb/remote: use remote_add_thread from extended_remote_target::attach

Message ID 371f552173633f4e933a4bbfa9f3dc70cc3f284b.1623268999.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • gdb/remote: spot stop packets sent for the wrong thread
Related show

Commit Message

Andrew Burgess June 9, 2021, 8:06 p.m.
When attaching we currently call add_thread_silent directly, rather
than calling remote_add_thread.  The problem this causes me is that in
remote_add_thread we set the new threads remote_state to RESUMED (new
threads are assumed to be running), but we don't do this in the attach
case.

I believe that this was just an oversight when the new resumed_state
mechanism was introduced.

In a later commit I would like to check that stop packets only arrive
from the remote for threads that are resumed.  As attached threads are
not marked as resumed we are left in a situation where the first stop
packet after an attach arrives for a thread that is not marked as
resumed.

In this commit I have the ::attach code call ::remote_add_thread, this
means that the new thread is correctly marked as resumed.

The only problem is that ::remote_add_thread only calls
add_thread_silent if we are in the process of initialising the remote
target, otherwise it calls add_thread, this would be a change in
behaviour.

To avoid this I considered setting remote_state::starting_up within
the ::attach code, however, this would mean that a Ctrl-C during the
attach would also change its behaviour, which is not what we want.

And so, in this commit, I add a new remote_state::attaching flag,
which is set to true for the duration of ::attach, this flag is then
checked in ::remote_add_thread just like remote_state::starting_up,
and causes GDB to call add_thread_silent.

There's no test for this change, but, after the _next_ commit, if this
change is _not_ included, then we see failures in
gdb.server/ext-attach.exp.

There should be no user visible changes after this commit.

gdb/ChangeLog:

	* remote.c (remote_state) <attaching>: New member variable.
	(remote_target::remote_add_thread): Check the attaching flag too.
	(extended_remote_target::attach): Set remote_state::attaching true
	for the scope of this function, and use remote_add_thread.
---
 gdb/ChangeLog |  7 +++++++
 gdb/remote.c  | 20 +++++++++++++-------
 2 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.25.4

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 97268151a59..85ab16e894e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -252,6 +252,9 @@  class remote_state
      about the remote side's threads, relocating symbols, etc.).  */
   bool starting_up = false;
 
+  /* True if we are going through the ::attach code.  */
+  bool attaching = false;
+
   /* If we negotiated packet size explicitly (and thus can bypass
      heuristics for the largest packet size that will not overflow
      a buffer in the stub), this will be set to that packet size.
@@ -2536,7 +2539,7 @@  remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing)
      consider that a single-threaded target, mentioning a new thread
      might be confusing to the user.  Be silent then, preserving the
      age old behavior.  */
-  if (rs->starting_up)
+  if (rs->starting_up || rs->attaching)
     thread = add_thread_silent (this, ptid);
   else
     thread = add_thread (this, ptid);
@@ -5977,6 +5980,12 @@  extended_remote_target::attach (const char *args, int from_tty)
   int pid;
   char *wait_status = NULL;
 
+  /* Let ::remote-add_thread know we are going through the initial attach
+     process, and so should not announce the first thread to the user.  */
+  gdb_assert (!rs->attaching);
+  scoped_restore attaching_restore
+    = make_scoped_restore (&rs->attaching, true);
+
   pid = parse_pid_to_attach (args);
 
   /* Remote PID can be freely equal to getpid, do not check it here the same
@@ -6045,14 +6054,11 @@  extended_remote_target::attach (const char *args, int from_tty)
 	 ptid.  */
       ptid_t curr_ptid = remote_current_thread (ptid_t (pid));
 
-      /* Add the main thread to the thread list.  */
-      thread_info *thr = add_thread_silent (this, curr_ptid);
+      /* Add the main thread to the thread list, this thread is initially
+	 assumed to be executing/running.  */
+      thread_info *thr = remote_add_thread (curr_ptid, true, true);
 
       switch_to_thread (thr);
-
-      /* Don't consider the thread stopped until we've processed the
-	 saved stop reply.  */
-      set_executing (this, thr->ptid, true);
     }
 
   /* Next, if the target can specify a description, read it.  We do