Make the remote target always register a thread

Make the remote target always register a thread

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

Hi,

This patch makes the remote target always put a thread in the
thread list.  This is needed so we can get rid of context
switching on the core side.

This is also the fix I suggested for the crash reported in this thread:
"How to catch GDB crash"
http://sourceware.org/ml/gdb/2008-06/msg00242.html

(The crash will still be there for other targets that
don't always register the main thread)

The remote target is special in its handling of threads,
as depending on the optional thread related packets supported,
inferior_ptid can "upgrade" from magic_null_ptid to a
real ptid, hence care must be taken to not leave an old
thread in the thread list with magic_null_ptid ptid, if
threads are reported.

I've tested those special cases manually by hacking GDB,
so I could test combinations like,

target remote, no qC support, but qfThreadInfo supported,
target remote, no qC support, but T AA thread:TID is reported,

target extended-remote, attaching, with no threads support (no
qC, or qfThreadInfo or T AA thread:TID, or the old thread listing
protocol)

target extended-remote, running, again with no threads support.

All the above, but after connecting, enabling T AA thread:TID
All the above, but after connecting, enabling qC

GDB behaved correctly, in those cases, that is, the <main>
thread is upgraded to a real ptid correctly.

The start_remote change is needed, because we're
adding the thread to the list before calling it, so we can't
call init_thread_list there.  Instead the call is moved to the
start_remote called.  There's another caller in monitor.c,
so that also gets the obvious change.

Regtested with a native gdbserver on x86_64-unknown-linux-gnu.

OK?

-- 
Pedro Alves

[-- Attachment #2: 002-remote_always_a_thread.diff --]
[-- Type: text/x-diff, Size: 6028 bytes --]

2008-06-25  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (start_remote): Don't clear thread list here.
	* monitor.c (monitor_open): Clear thread list here.
	
	* remote.c (record_currthread): Upgrade the main thread and its
	entry in the thread list if this is the first time we hear about
	threads.
	(remote_thread_alive): Consider magic_null_ptid or a ptid without
	a tid member always alive.
	(remote_find_new_threads): Don't update the main thread here.
	(remote_start_remote): Clear thread list here.  Always add the
	main thread.
	(extended_remote_attach_1): Add the main thread here.
	(extended_remote_mourn_1): Re-add the main thread here.
	(extended_remote_create_inferior_1): Add a main thread.

---
 gdb/infrun.c  |    1 -
 gdb/monitor.c |    3 +++
 gdb/remote.c  |   54 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 52 insertions(+), 6 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-06-25 20:09:25.000000000 +0100
+++ src/gdb/infrun.c	2008-06-25 21:13:35.000000000 +0100
@@ -1281,7 +1281,6 @@ proceed (CORE_ADDR addr, enum target_sig
 void
 start_remote (int from_tty)
 {
-  init_thread_list ();
   init_wait_for_inferior ();
   stop_soon = STOP_QUIETLY_REMOTE;
   stepping_over_breakpoint = 0;
Index: src/gdb/monitor.c
===================================================================
--- src.orig/gdb/monitor.c	2008-06-25 20:09:25.000000000 +0100
+++ src/gdb/monitor.c	2008-06-25 21:13:35.000000000 +0100
@@ -804,6 +804,9 @@ monitor_open (char *args, struct monitor
 
   push_target (targ_ops);
 
+  /* Start afresh.  */
+  init_thread_list ();
+
   inferior_ptid = pid_to_ptid (42000);	/* Make run command think we are busy... */
 
   /* Give monitor_wait something to read */
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-06-25 21:13:34.000000000 +0100
+++ src/gdb/remote.c	2008-06-25 22:34:48.000000000 +0100
@@ -1089,7 +1089,31 @@ record_currthread (ptid_t currthread)
   /* If this is a new thread, add it to GDB's thread list.
      If we leave it up to WFI to do this, bad things will happen.  */
   if (!in_thread_list (currthread))
-    add_thread (currthread);
+    {
+      if (ptid_equal (pid_to_ptid (ptid_get_pid (currthread)), inferior_ptid))
+	{
+	  /* inferior_ptid has no thread member yet.  This can happen
+	     with the vAttach -> remote_wait,"TAAthread:" path if the
+	     stub doesn't support qC.  This is the first stop reported
+	     after an attach, so this is the main thread.  Update the
+	     ptid in the thread list.  */
+	  struct thread_info *th = find_thread_pid (inferior_ptid);
+	  inferior_ptid = th->ptid = currthread;
+	}
+      else if (ptid_equal (magic_null_ptid, inferior_ptid))
+	{
+	  /* inferior_ptid is not set yet.  This can happen with the
+	     vRun -> remote_wait,"TAAthread:" path if the stub
+	     doesn't support qC.  This is the first stop reported
+	     after an attach, so this is the main thread.  Update the
+	     ptid in the thread list.  */
+	  struct thread_info *th = find_thread_pid (inferior_ptid);
+	  inferior_ptid = th->ptid = currthread;
+	}
+      else
+	/* This is really a new thread.  Add it.  */
+	add_thread (currthread);
+    }
 }
 
 static char *last_pass_packet;
@@ -1212,6 +1236,16 @@ remote_thread_alive (ptid_t ptid)
   struct remote_state *rs = get_remote_state ();
   int tid = ptid_get_tid (ptid);
 
+  if (ptid_equal (ptid, magic_null_ptid))
+    /* The main thread is always alive.  */
+    return 1;
+
+  if (ptid_get_pid (ptid) != 0 && ptid_get_tid (ptid) == 0)
+    /* The main thread is always alive.  This can happen after a
+       vAttach, if the remote side doesn't support
+       multi-threading.  */
+    return 1;
+
   if (tid < 0)
     xsnprintf (rs->buf, get_remote_packet_size (), "T-%08x", -tid);
   else
@@ -1925,9 +1959,6 @@ remote_find_new_threads (void)
 {
   remote_threadlist_iterator (remote_newthread_step, 0,
 			      CRAZY_MAX_THREADS);
-  if (ptid_equal (inferior_ptid, magic_null_ptid))
-    /* We don't know the current thread yet.  Query it.  */
-    inferior_ptid = remote_current_thread (inferior_ptid);
 }
 
 /*
@@ -2289,6 +2320,9 @@ remote_start_remote (struct ui_out *uiou
       strcpy (wait_status, rs->buf);
     }
 
+  /* Start afresh.  */
+  init_thread_list ();
+
   /* Let the stub know that we want it to return the thread.  */
   set_continue_thread (minus_one_ptid);
 
@@ -2304,6 +2338,9 @@ remote_start_remote (struct ui_out *uiou
   /* Now, if we have thread information, update inferior_ptid.  */
   inferior_ptid = remote_current_thread (inferior_ptid);
 
+  /* Always add the main thread.  */
+  add_thread_silent (inferior_ptid);
+
   get_offsets ();		/* Get text, data & bss offsets.  */
 
   /* Use the previously fetched status.  */
@@ -2934,6 +2971,9 @@ extended_remote_attach_1 (struct target_
   /* Now, if we have thread information, update inferior_ptid.  */
   inferior_ptid = remote_current_thread (inferior_ptid);
 
+  /* Now, add the main thread to the thread list.  */
+  add_thread_silent (inferior_ptid);
+
   attach_flag = 1;
 
   /* Next, if the target can specify a description, read it.  We do
@@ -5152,7 +5192,8 @@ extended_remote_mourn_1 (struct target_o
       /* Assume that the target has been restarted.  Set inferior_ptid
 	 so that bits of core GDB realizes there's something here, e.g.,
 	 so that the user can say "kill" again.  */
-      inferior_ptid = magic_null_ptid;
+      inferior_ptid = remote_current_thread (magic_null_ptid);
+      add_thread_silent (inferior_ptid);
     }
   else
     {
@@ -5267,6 +5308,9 @@ extended_remote_create_inferior_1 (char 
   /* Now mark the inferior as running before we do anything else.  */
   attach_flag = 0;
   inferior_ptid = magic_null_ptid;
+
+  add_thread_silent (inferior_ptid);
+
   target_mark_running (&extended_remote_ops);
 
   /* Get updated offsets, if the stub uses qOffsets.  */

Re: Make the remote target always register a thread

[Pedro Alves]

A Thursday 26 June 2008 00:08:08, Pedro Alves wrote:
> This patch makes the remote target always put a thread in the
> thread list.  This is needed so we can get rid of context
> switching on the core side.
>

Sorry, forgot to add that this patch applies on top of the other
one I just sent that makes the remote target use ptid.tid for
thread ids.

[make the remote target store thread ids in ptid_t.tid]
http://sourceware.org/ml/gdb-patches/2008-06/msg00458.html

-- 
Pedro Alves

Re: Make the remote target always register a thread

[Daniel Jacobowitz]

On Thu, Jun 26, 2008 at 12:08:08AM +0100, Pedro Alves wrote:
> (The crash will still be there for other targets that
> don't always register the main thread)

Be nice to get rid of the crash, one way or another.

> 2008-06-25  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* infrun.c (start_remote): Don't clear thread list here.
> 	* monitor.c (monitor_open): Clear thread list here.
> 	
> 	* remote.c (record_currthread): Upgrade the main thread and its
> 	entry in the thread list if this is the first time we hear about
> 	threads.
> 	(remote_thread_alive): Consider magic_null_ptid or a ptid without
> 	a tid member always alive.
> 	(remote_find_new_threads): Don't update the main thread here.
> 	(remote_start_remote): Clear thread list here.  Always add the
> 	main thread.
> 	(extended_remote_attach_1): Add the main thread here.
> 	(extended_remote_mourn_1): Re-add the main thread here.
> 	(extended_remote_create_inferior_1): Add a main thread.

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Make the remote target always register a thread

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

A Thursday 26 June 2008 03:48:14, Daniel Jacobowitz wrote:
> On Thu, Jun 26, 2008 at 12:08:08AM +0100, Pedro Alves wrote:
> > (The crash will still be there for other targets that
> > don't always register the main thread)
>
> Be nice to get rid of the crash, one way or another.

It will take a while to make all targets consistent,
so although I'd prefer to fix them all, practically speaking, the
assertion should be removed for now, and perhaps leave a comment
there so when we do have all targets consistent, the assert will be
catching a real bug.

The fix seems to me to be a one or two liner,
but, as I've said at gdb@, I don't know if we should output
thread-id=0 or not output thread-id at all, or maybe even
output thread-id=all, as if there are no threads in the list,
this is the special case of there being only one "task" really,
but the target is not registering it.

(OTOH, having an assert at least makes it easy to see
which target needs to be changed.)

> OK.

Thanks, I've checked it in, with an obvious change as attached.  I needed
to include gdbthread.h in monitor.c.

-- 
Pedro Alves

[-- Attachment #2: 002-remote_always_a_thread.diff --]
[-- Type: text/x-diff, Size: 7137 bytes --]

2008-06-27  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (start_remote): Don't clear thread list here.
	* monitor.c (monitor_open): Include "gdbthread.h".  Clear thread
	list here.
	* remote.c (record_currthread): Upgrade the main thread and its
	entry in the thread list if this is the first time we hear about
	threads.
	(remote_thread_alive): Consider magic_null_ptid or a ptid without
	a tid member always alive.
	(remote_find_new_threads): Don't update the main thread here.
	(remote_start_remote): Clear thread list here.  Always add the
	main thread.
	(extended_remote_attach_1): Add the main thread here.
	(extended_remote_mourn_1): Re-add the main thread here.
	(extended_remote_create_inferior_1): Add a main thread.

	* Makefile.in (monitor.o): Depend on $(gdbthread_h).

---
 gdb/Makefile.in |    2 +-
 gdb/infrun.c    |    1 -
 gdb/monitor.c   |    4 ++++
 gdb/remote.c    |   54 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 54 insertions(+), 7 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-06-27 12:30:13.000000000 +0100
+++ src/gdb/infrun.c	2008-06-27 12:33:21.000000000 +0100
@@ -1281,7 +1281,6 @@ proceed (CORE_ADDR addr, enum target_sig
 void
 start_remote (int from_tty)
 {
-  init_thread_list ();
   init_wait_for_inferior ();
   stop_soon = STOP_QUIETLY_REMOTE;
   stepping_over_breakpoint = 0;
Index: src/gdb/monitor.c
===================================================================
--- src.orig/gdb/monitor.c	2008-06-27 12:30:13.000000000 +0100
+++ src/gdb/monitor.c	2008-06-27 12:37:31.000000000 +0100
@@ -54,6 +54,7 @@
 #include "gdb_regex.h"
 #include "srec.h"
 #include "regcache.h"
+#include "gdbthread.h"
 
 static char *dev_name;
 static struct target_ops *targ_ops;
@@ -804,6 +805,9 @@ monitor_open (char *args, struct monitor
 
   push_target (targ_ops);
 
+  /* Start afresh.  */
+  init_thread_list ();
+
   inferior_ptid = pid_to_ptid (42000);	/* Make run command think we are busy... */
 
   /* Give monitor_wait something to read */
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-06-27 12:33:04.000000000 +0100
+++ src/gdb/remote.c	2008-06-27 12:33:21.000000000 +0100
@@ -1089,7 +1089,31 @@ record_currthread (ptid_t currthread)
   /* If this is a new thread, add it to GDB's thread list.
      If we leave it up to WFI to do this, bad things will happen.  */
   if (!in_thread_list (currthread))
-    add_thread (currthread);
+    {
+      if (ptid_equal (pid_to_ptid (ptid_get_pid (currthread)), inferior_ptid))
+	{
+	  /* inferior_ptid has no thread member yet.  This can happen
+	     with the vAttach -> remote_wait,"TAAthread:" path if the
+	     stub doesn't support qC.  This is the first stop reported
+	     after an attach, so this is the main thread.  Update the
+	     ptid in the thread list.  */
+	  struct thread_info *th = find_thread_pid (inferior_ptid);
+	  inferior_ptid = th->ptid = currthread;
+	}
+      else if (ptid_equal (magic_null_ptid, inferior_ptid))
+	{
+	  /* inferior_ptid is not set yet.  This can happen with the
+	     vRun -> remote_wait,"TAAthread:" path if the stub
+	     doesn't support qC.  This is the first stop reported
+	     after an attach, so this is the main thread.  Update the
+	     ptid in the thread list.  */
+	  struct thread_info *th = find_thread_pid (inferior_ptid);
+	  inferior_ptid = th->ptid = currthread;
+	}
+      else
+	/* This is really a new thread.  Add it.  */
+	add_thread (currthread);
+    }
 }
 
 static char *last_pass_packet;
@@ -1212,6 +1236,16 @@ remote_thread_alive (ptid_t ptid)
   struct remote_state *rs = get_remote_state ();
   int tid = ptid_get_tid (ptid);
 
+  if (ptid_equal (ptid, magic_null_ptid))
+    /* The main thread is always alive.  */
+    return 1;
+
+  if (ptid_get_pid (ptid) != 0 && ptid_get_tid (ptid) == 0)
+    /* The main thread is always alive.  This can happen after a
+       vAttach, if the remote side doesn't support
+       multi-threading.  */
+    return 1;
+
   if (tid < 0)
     xsnprintf (rs->buf, get_remote_packet_size (), "T-%08x", -tid);
   else
@@ -1925,9 +1959,6 @@ remote_find_new_threads (void)
 {
   remote_threadlist_iterator (remote_newthread_step, 0,
 			      CRAZY_MAX_THREADS);
-  if (ptid_equal (inferior_ptid, magic_null_ptid))
-    /* We don't know the current thread yet.  Query it.  */
-    inferior_ptid = remote_current_thread (inferior_ptid);
 }
 
 /*
@@ -2289,6 +2320,9 @@ remote_start_remote (struct ui_out *uiou
       strcpy (wait_status, rs->buf);
     }
 
+  /* Start afresh.  */
+  init_thread_list ();
+
   /* Let the stub know that we want it to return the thread.  */
   set_continue_thread (minus_one_ptid);
 
@@ -2304,6 +2338,9 @@ remote_start_remote (struct ui_out *uiou
   /* Now, if we have thread information, update inferior_ptid.  */
   inferior_ptid = remote_current_thread (inferior_ptid);
 
+  /* Always add the main thread.  */
+  add_thread_silent (inferior_ptid);
+
   get_offsets ();		/* Get text, data & bss offsets.  */
 
   /* Use the previously fetched status.  */
@@ -2934,6 +2971,9 @@ extended_remote_attach_1 (struct target_
   /* Now, if we have thread information, update inferior_ptid.  */
   inferior_ptid = remote_current_thread (inferior_ptid);
 
+  /* Now, add the main thread to the thread list.  */
+  add_thread_silent (inferior_ptid);
+
   attach_flag = 1;
 
   /* Next, if the target can specify a description, read it.  We do
@@ -5152,7 +5192,8 @@ extended_remote_mourn_1 (struct target_o
       /* Assume that the target has been restarted.  Set inferior_ptid
 	 so that bits of core GDB realizes there's something here, e.g.,
 	 so that the user can say "kill" again.  */
-      inferior_ptid = magic_null_ptid;
+      inferior_ptid = remote_current_thread (magic_null_ptid);
+      add_thread_silent (inferior_ptid);
     }
   else
     {
@@ -5267,6 +5308,9 @@ extended_remote_create_inferior_1 (char 
   /* Now mark the inferior as running before we do anything else.  */
   attach_flag = 0;
   inferior_ptid = magic_null_ptid;
+
+  add_thread_silent (inferior_ptid);
+
   target_mark_running (&extended_remote_ops);
 
   /* Get updated offsets, if the stub uses qOffsets.  */
Index: src/gdb/Makefile.in
===================================================================
--- src.orig/gdb/Makefile.in	2008-06-27 12:37:12.000000000 +0100
+++ src/gdb/Makefile.in	2008-06-27 12:38:20.000000000 +0100
@@ -2540,7 +2540,7 @@ mn10300-tdep.o: mn10300-tdep.c $(defs_h)
 	$(symtab_h) $(dwarf2_frame_h) $(osabi_h) $(target_h) $(mn10300_tdep_h)
 monitor.o: monitor.c $(defs_h) $(gdbcore_h) $(target_h) $(exceptions_h) \
 	$(gdb_string_h) $(command_h) $(serial_h) $(monitor_h) $(gdbcmd_h) \
-	$(inferior_h) $(gdb_regex_h) $(srec_h) $(regcache_h)
+	$(inferior_h) $(gdb_regex_h) $(srec_h) $(regcache_h) $(gdbthread_h)
 mt-tdep.o: mt-tdep.c $(defs_h) $(frame_h) $(frame_unwind_h) $(frame_base_h) \
 	$(symtab_h) $(dis_asm_h) $(arch_utils_h) $(gdbtypes_h) \
 	$(gdb_string_h) $(regcache_h) $(reggroups_h) $(gdbcore_h) \

Re: Make the remote target always register a thread

[Daniel Jacobowitz]

On Fri, Jun 27, 2008 at 12:58:08PM +0100, Pedro Alves wrote:
> It will take a while to make all targets consistent,
> so although I'd prefer to fix them all, practically speaking, the
> assertion should be removed for now, and perhaps leave a comment
> there so when we do have all targets consistent, the assert will be
> catching a real bug.
> 
> The fix seems to me to be a one or two liner,
> but, as I've said at gdb@, I don't know if we should output
> thread-id=0 or not output thread-id at all, or maybe even
> output thread-id=all, as if there are no threads in the list,
> this is the special case of there being only one "task" really,
> but the target is not registering it.
> 
> (OTOH, having an assert at least makes it easy to see
> which target needs to be changed.)

Let's just keep in mind that we shouldn't ship a release of GDB with
this problem...

-- 
Daniel Jacobowitz
CodeSourcery