[1/7] Register the main thread/task in fork-child.c

[1/7] Register the main thread/task in fork-child.c

[Pedro Alves]

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

Hi,

This patch makes it so that right after a fork-child, we add the main
task of the inferior to GDB's thread tables.  We don't have lwp or thread
info at this point, which means that targets should decorate more of
inferior_ptid as soon as they have the chance.  Some targets will do it
as soon as we get to the first target_wait, others, will only have
info available when a thread library is loaded.

The issue of changing inferior_ptid to accomodate a new ptid that
represents the same task, in GDB's perspective is not new.  See below for
examples are all over the place.

Since we're now making sure inferior_ptid is always in the thread list,
when we update it to include more lwp or tid info, we also need to make
sure that entry in the thread list is updated.  In addition, if there are
other GDB's sub-components that were holding info on this thread, we should
be able to inform them of the ptid change.  Hence, I'm adding a new
thread_change_ptid function, which takes care of updating the thread table,
and a new observer that is called by this function, so other modules can
react:

 @deftypefun void thread_ptid_changed (ptid_t @var{old_ptid}, ptid_t   
 @var{new_ptid}) 
 The thread's ptid has changed.  The @var{old_ptid} parameter specifies
 the old value, and @var{new_ptid} specifies the new value.
 @end deftypefun

I'm including the linux-nat.c change in this patch, for an example usable.

The following patches in the series will introduce more uses.

The whole series was tested on x86_64-unknown-linux-gnu, i386-pc-solaris2.11
(OpenSolaris 10), i386-unknown-openbsd4.3, i386-unknown-freebsd6.0,
i386-unknown-freebsd7.0 and i686-unknown-gnu0.3 (Debian GNU/Hurd).

OK, when the rest of the series is OK?

--------

infrun.c:

	  /* The call to in_thread_list is necessary because PTIDs sometimes
	     change when we go from single-threaded to multi-threaded.  If
	     the singlestep_ptid is still in the list, assume that it is
	     really different from ecs->ptid.  */
	  if (!ptid_equal (singlestep_ptid, ecs->ptid)
	      && in_thread_list (singlestep_ptid))
	    {

bsd-uthread.c:

  /* HACK: Twiddle INFERIOR_PTID such that the initial thread of a
     process isn't recognized as a new thread.  */
  if (ptid_get_tid (ptid) != 0 && !in_thread_list (ptid)
      && ptid_get_tid (inferior_ptid) == 0)
    {
      add_thread_silent (ptid);
      inferior_ptid = ptid;
    }

linux-nat.c:

  /* The first time we get here after starting a new inferior, we may
     not have added it to the LWP list yet - this is the earliest
     moment at which we know its PID.  */
  if (num_lwps == 0)
    {
      gdb_assert (!is_lwp (inferior_ptid));

      inferior_ptid = BUILD_LWP (GET_PID (inferior_ptid),
				 GET_PID (inferior_ptid));

inf-trace.c:

 /* HACK: Twiddle INFERIOR_PTID such that the initial thread of a
     process isn't recognized as a new thread.  */
  if (ptid_get_lwp (inferior_ptid) == 0)
    inferior_ptid = ptid;


-- 
Pedro Alves

[-- Attachment #2: 001-always_a_thread_fork.diff --]
[-- Type: text/x-diff, Size: 6955 bytes --]

2008-08-08  Pedro Alves  <pedro@codesourcery.com>

	gdb/doc/
	* observer.texi (thread_ptid_changed): New.

	gdb/
	* gdbthread.h (thread_change_ptid): Declare.
	* infrun.c (infrun_thread_ptid_changed): New.
	(_initialize_infrun): Attach infrun_thread_ptid_changed to the
	thread_ptid_changed observer.
	* linux-nat.c (linux_nat_wait): Update inferior_ptid's ptid with
	thread_change_ptid.  Don't add or mark the main thread as running
	and executing here.
	* regcache.c (regcache_thread_ptid_changed): New.
	(_initialize_regcache): Attach regcache_thread_ptid_changed to the
	thread_ptid_changed observer.
	* thread.c (thread_change_ptid): New.
	* fork-child.c (fork_inferior): Add the main thread here, and set
	it running and executing.

---
 gdb/doc/observer.texi |    4 ++++
 gdb/fork-child.c      |    6 ++++++
 gdb/gdbthread.h       |    3 +++
 gdb/infrun.c          |   26 ++++++++++++++++++++++++++
 gdb/linux-nat.c       |   11 +++++------
 gdb/regcache.c        |   11 +++++++++++
 gdb/thread.c          |    9 +++++++++
 7 files changed, 64 insertions(+), 6 deletions(-)

Index: src/gdb/doc/observer.texi
===================================================================
--- src.orig/gdb/doc/observer.texi	2008-08-07 17:20:10.000000000 +0100
+++ src/gdb/doc/observer.texi	2008-08-07 19:34:53.000000000 +0100
@@ -175,3 +175,7 @@ The current architecture has changed.  T
 a pointer to the new architecture.
 @end deftypefun
 
+@deftypefun void thread_ptid_changed (ptid_t @var{old_ptid}, ptid_t @var{new_ptid})
+The thread's ptid has changed.  The @var{old_ptid} parameter specifies
+the old value, and @var{new_ptid} specifies the new value.
+@end deftypefun
Index: src/gdb/gdbthread.h
===================================================================
--- src.orig/gdb/gdbthread.h	2008-08-07 17:23:21.000000000 +0100
+++ src/gdb/gdbthread.h	2008-08-07 17:23:29.000000000 +0100
@@ -151,6 +151,9 @@ extern struct thread_info *find_thread_p
 /* Find thread by GDB user-visible thread number.  */
 struct thread_info *find_thread_id (int num);
 
+/* Change the ptid of thread OLD_PTID to NEW_PTID.  */
+void thread_change_ptid (ptid_t old_ptid, ptid_t new_ptid);
+
 /* Iterator function to call a user-provided callback function
    once for each known thread.  */
 typedef int (*thread_callback_func) (struct thread_info *, void *);
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-08-07 17:20:10.000000000 +0100
+++ src/gdb/infrun.c	2008-08-07 17:23:29.000000000 +0100
@@ -865,6 +865,30 @@ displaced_step_fixup (ptid_t event_ptid,
     }
 }
 
+/* Update global variables old ptids to hold NEW_PTID if they were
+   holding OLD_PTID.  */
+static void
+infrun_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
+{
+  struct displaced_step_request *it;
+
+  if (ptid_equal (inferior_ptid, old_ptid))
+    inferior_ptid = new_ptid;
+
+  if (ptid_equal (singlestep_ptid, old_ptid))
+    singlestep_ptid = new_ptid;
+
+  if (ptid_equal (displaced_step_ptid, old_ptid))
+    displaced_step_ptid = new_ptid;
+
+  if (ptid_equal (deferred_step_ptid, old_ptid))
+    deferred_step_ptid = new_ptid;
+
+  for (it = displaced_step_request_queue; it; it = it->next)
+    if (ptid_equal (it->ptid, old_ptid))
+      it->ptid = new_ptid;
+}
+
 \f
 /* Resuming.  */
 
@@ -4853,4 +4877,6 @@ breakpoints, even if such is supported b
   inferior_ptid = null_ptid;
   target_last_wait_ptid = minus_one_ptid;
   displaced_step_ptid = null_ptid;
+
+  observer_attach_thread_ptid_changed (infrun_thread_ptid_changed);
 }
Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2008-08-07 17:20:10.000000000 +0100
+++ src/gdb/linux-nat.c	2008-08-07 19:30:21.000000000 +0100
@@ -2722,14 +2722,13 @@ linux_nat_wait (ptid_t ptid, struct targ
     {
       gdb_assert (!is_lwp (inferior_ptid));
 
-      inferior_ptid = BUILD_LWP (GET_PID (inferior_ptid),
-				 GET_PID (inferior_ptid));
+      /* Upgrade the main thread's ptid.  */
+      thread_change_ptid (inferior_ptid,
+			  BUILD_LWP (GET_PID (inferior_ptid),
+				     GET_PID (inferior_ptid)));
+
       lp = add_lwp (inferior_ptid);
       lp->resumed = 1;
-      /* Add the main thread to GDB's thread list.  */
-      add_thread_silent (lp->ptid);
-      set_running (lp->ptid, 1);
-      set_executing (lp->ptid, 1);
     }
 
   /* Block events while we're here.  */
Index: src/gdb/regcache.c
===================================================================
--- src.orig/gdb/regcache.c	2008-08-07 17:20:10.000000000 +0100
+++ src/gdb/regcache.c	2008-08-07 17:23:29.000000000 +0100
@@ -453,6 +453,16 @@ regcache_observer_target_changed (struct
   registers_changed ();
 }
 
+/* Update global variables old ptids to hold NEW_PTID if they were
+   holding OLD_PTID.  */
+static void
+regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
+{
+  if (current_regcache != NULL
+      && ptid_equal (current_regcache->ptid, old_ptid))
+    current_regcache->ptid = new_ptid;
+}
+
 /* Low level examining and depositing of registers.
 
    The caller is responsible for making sure that the inferior is
@@ -1134,6 +1144,7 @@ _initialize_regcache (void)
   regcache_descr_handle = gdbarch_data_register_post_init (init_regcache_descr);
 
   observer_attach_target_changed (regcache_observer_target_changed);
+  observer_attach_thread_ptid_changed (regcache_thread_ptid_changed);
 
   add_com ("flushregs", class_maintenance, reg_flush_command,
 	   _("Force gdb to flush its register cache (maintainer command)"));
Index: src/gdb/thread.c
===================================================================
--- src.orig/gdb/thread.c	2008-08-07 17:20:10.000000000 +0100
+++ src/gdb/thread.c	2008-08-07 17:23:29.000000000 +0100
@@ -589,6 +589,15 @@ prune_threads (void)
 }
 
 void
+thread_change_ptid (ptid_t old_ptid, ptid_t new_ptid)
+{
+  struct thread_info * tp = find_thread_pid (old_ptid);
+  tp->ptid = new_ptid;
+
+  observer_notify_thread_ptid_changed (old_ptid, new_ptid);
+}
+
+void
 set_running (ptid_t ptid, int running)
 {
   struct thread_info *tp;
Index: src/gdb/fork-child.c
===================================================================
--- src.orig/gdb/fork-child.c	2008-08-07 17:20:10.000000000 +0100
+++ src/gdb/fork-child.c	2008-08-07 17:23:29.000000000 +0100
@@ -397,6 +397,12 @@ fork_inferior (char *exec_file_arg, char
   /* Needed for wait_for_inferior stuff below.  */
   inferior_ptid = pid_to_ptid (pid);
 
+  /* We have something that executes now.  We'll be running through
+     the shell at this point, but the pid shouldn't change.  Targets
+     supporting MT should fill this task's ptid with more data as soon
+     as they can.  */
+  add_thread_silent (inferior_ptid);
+
   /* Now that we have a child process, make it our target, and
      initialize anything target-vector-specific that needs
      initializing.  */

Re: [1/7] Register the main thread/task in fork-child.c

[Mark Kettenis]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 8 Aug 2008 02:33:48 +0100
> 
> Hi,
> 
> This patch makes it so that right after a fork-child, we add the main
> task of the inferior to GDB's thread tables.  We don't have lwp or thread
> info at this point, which means that targets should decorate more of
> inferior_ptid as soon as they have the chance.  Some targets will do it
> as soon as we get to the first target_wait, others, will only have
> info available when a thread library is loaded.
> 
> The issue of changing inferior_ptid to accomodate a new ptid that
> represents the same task, in GDB's perspective is not new.  See below for
> examples are all over the place.
> 
> Since we're now making sure inferior_ptid is always in the thread list,
> when we update it to include more lwp or tid info, we also need to make
> sure that entry in the thread list is updated.  In addition, if there are
> other GDB's sub-components that were holding info on this thread, we should
> be able to inform them of the ptid change.  Hence, I'm adding a new
> thread_change_ptid function, which takes care of updating the thread table,
> and a new observer that is called by this function, so other modules can
> react:
> 
>  @deftypefun void thread_ptid_changed (ptid_t @var{old_ptid}, ptid_t   
>  @var{new_ptid}) 
>  The thread's ptid has changed.  The @var{old_ptid} parameter specifies
>  the old value, and @var{new_ptid} specifies the new value.
>  @end deftypefun
> 
> OK, when the rest of the series is OK?

Heh, I had the exact same fork-child.c change in my tree, but didn't
post a patch yet since I didn't check out yet how this affected Linux.
So that bit must be ok!

The observer makes sense to me too.  So the overall answer is yes!

I'll try to look at the bsd-uthread diff ASAP.


> 2008-08-08  Pedro Alves  <pedro@codesourcery.com>
> 
> 	gdb/doc/
> 	* observer.texi (thread_ptid_changed): New.
> 
> 	gdb/
> 	* gdbthread.h (thread_change_ptid): Declare.
> 	* infrun.c (infrun_thread_ptid_changed): New.
> 	(_initialize_infrun): Attach infrun_thread_ptid_changed to the
> 	thread_ptid_changed observer.
> 	* linux-nat.c (linux_nat_wait): Update inferior_ptid's ptid with
> 	thread_change_ptid.  Don't add or mark the main thread as running
> 	and executing here.
> 	* regcache.c (regcache_thread_ptid_changed): New.
> 	(_initialize_regcache): Attach regcache_thread_ptid_changed to the
> 	thread_ptid_changed observer.
> 	* thread.c (thread_change_ptid): New.
> 	* fork-child.c (fork_inferior): Add the main thread here, and set
> 	it running and executing.

Re: [1/7] Register the main thread/task in fork-child.c

[Pedro Alves]

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

On Friday 08 August 2008 22:45:52, Mark Kettenis wrote:

> The observer makes sense to me too.  So the overall answer is yes!

Thanks!

I took the liberty of splitting this patch in two, as there
are other patches that are ready to be checked in, that
depended on thread_change_ptid, but which do not depend on
fork-child.c.

Attached is what I checked in.

-- 
Pedro Alves

[-- Attachment #2: thread_change_ptid.diff --]
[-- Type: text/x-diff, Size: 4944 bytes --]

2008-08-18  Pedro Alves  <pedro@codesourcery.com>

	gdb/doc/
	* observer.texi (thread_ptid_changed): New.

	gdb/
	* gdbthread.h (thread_change_ptid): Declare.
	* infrun.c (infrun_thread_ptid_changed): New.
	(_initialize_infrun): Attach infrun_thread_ptid_changed to the
	thread_ptid_changed observer.
	* regcache.c (regcache_thread_ptid_changed): New.
	(_initialize_regcache): Attach regcache_thread_ptid_changed to the
	thread_ptid_changed observer.
	* thread.c (thread_change_ptid): New.

---
 gdb/doc/observer.texi |    4 ++++
 gdb/gdbthread.h       |    3 +++
 gdb/infrun.c          |   26 ++++++++++++++++++++++++++
 gdb/regcache.c        |   11 +++++++++++
 gdb/thread.c          |    9 +++++++++
 5 files changed, 53 insertions(+)

Index: src/gdb/doc/observer.texi
===================================================================
--- src.orig/gdb/doc/observer.texi	2008-08-18 23:19:58.000000000 +0100
+++ src/gdb/doc/observer.texi	2008-08-18 23:20:13.000000000 +0100
@@ -175,3 +175,7 @@ The current architecture has changed.  T
 a pointer to the new architecture.
 @end deftypefun
 
+@deftypefun void thread_ptid_changed (ptid_t @var{old_ptid}, ptid_t @var{new_ptid})
+The thread's ptid has changed.  The @var{old_ptid} parameter specifies
+the old value, and @var{new_ptid} specifies the new value.
+@end deftypefun
Index: src/gdb/gdbthread.h
===================================================================
--- src.orig/gdb/gdbthread.h	2008-08-18 23:19:58.000000000 +0100
+++ src/gdb/gdbthread.h	2008-08-18 23:20:13.000000000 +0100
@@ -151,6 +151,9 @@ extern struct thread_info *find_thread_p
 /* Find thread by GDB user-visible thread number.  */
 struct thread_info *find_thread_id (int num);
 
+/* Change the ptid of thread OLD_PTID to NEW_PTID.  */
+void thread_change_ptid (ptid_t old_ptid, ptid_t new_ptid);
+
 /* Iterator function to call a user-provided callback function
    once for each known thread.  */
 typedef int (*thread_callback_func) (struct thread_info *, void *);
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-08-18 23:19:58.000000000 +0100
+++ src/gdb/infrun.c	2008-08-18 23:22:04.000000000 +0100
@@ -865,6 +865,30 @@ displaced_step_fixup (ptid_t event_ptid,
     }
 }
 
+/* Update global variables holding ptids to hold NEW_PTID if they were
+   holding OLD_PTID.  */
+static void
+infrun_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
+{
+  struct displaced_step_request *it;
+
+  if (ptid_equal (inferior_ptid, old_ptid))
+    inferior_ptid = new_ptid;
+
+  if (ptid_equal (singlestep_ptid, old_ptid))
+    singlestep_ptid = new_ptid;
+
+  if (ptid_equal (displaced_step_ptid, old_ptid))
+    displaced_step_ptid = new_ptid;
+
+  if (ptid_equal (deferred_step_ptid, old_ptid))
+    deferred_step_ptid = new_ptid;
+
+  for (it = displaced_step_request_queue; it; it = it->next)
+    if (ptid_equal (it->ptid, old_ptid))
+      it->ptid = new_ptid;
+}
+
 \f
 /* Resuming.  */
 
@@ -4855,4 +4879,6 @@ breakpoints, even if such is supported b
   inferior_ptid = null_ptid;
   target_last_wait_ptid = minus_one_ptid;
   displaced_step_ptid = null_ptid;
+
+  observer_attach_thread_ptid_changed (infrun_thread_ptid_changed);
 }
Index: src/gdb/regcache.c
===================================================================
--- src.orig/gdb/regcache.c	2008-08-18 23:19:58.000000000 +0100
+++ src/gdb/regcache.c	2008-08-18 23:20:13.000000000 +0100
@@ -453,6 +453,16 @@ regcache_observer_target_changed (struct
   registers_changed ();
 }
 
+/* Update global variables old ptids to hold NEW_PTID if they were
+   holding OLD_PTID.  */
+static void
+regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
+{
+  if (current_regcache != NULL
+      && ptid_equal (current_regcache->ptid, old_ptid))
+    current_regcache->ptid = new_ptid;
+}
+
 /* Low level examining and depositing of registers.
 
    The caller is responsible for making sure that the inferior is
@@ -1134,6 +1144,7 @@ _initialize_regcache (void)
   regcache_descr_handle = gdbarch_data_register_post_init (init_regcache_descr);
 
   observer_attach_target_changed (regcache_observer_target_changed);
+  observer_attach_thread_ptid_changed (regcache_thread_ptid_changed);
 
   add_com ("flushregs", class_maintenance, reg_flush_command,
 	   _("Force gdb to flush its register cache (maintainer command)"));
Index: src/gdb/thread.c
===================================================================
--- src.orig/gdb/thread.c	2008-08-18 23:19:58.000000000 +0100
+++ src/gdb/thread.c	2008-08-18 23:20:13.000000000 +0100
@@ -589,6 +589,15 @@ prune_threads (void)
 }
 
 void
+thread_change_ptid (ptid_t old_ptid, ptid_t new_ptid)
+{
+  struct thread_info * tp = find_thread_pid (old_ptid);
+  tp->ptid = new_ptid;
+
+  observer_notify_thread_ptid_changed (old_ptid, new_ptid);
+}
+
+void
 set_running (ptid_t ptid, int running)
 {
   struct thread_info *tp;

Re: [1/7] Register the main thread/task in fork-child.c

[Pedro Alves]

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

On Monday 18 August 2008 23:41:38, Pedro Alves wrote:
> On Friday 08 August 2008 22:45:52, Mark Kettenis wrote:
> > The observer makes sense to me too.  So the overall answer is yes!
>
> Thanks!
>
> I took the liberty of splitting this patch in two, as there
> are other patches that are ready to be checked in, that
> depended on thread_change_ptid, but which do not depend on
> fork-child.c.

> Attached is what I checked in.

And this is what I haven't checked in yet.

-- 
Pedro Alves

[-- Attachment #2: 001-always_a_thread_fork.diff --]
[-- Type: text/x-diff, Size: 2097 bytes --]

2008-08-18  Pedro Alves  <pedro@codesourcery.com>

	* linux-nat.c (linux_nat_wait): Update inferior_ptid's ptid with
	thread_change_ptid.  Don't add or mark the main thread as running
	and executing here.
	* fork-child.c (fork_inferior): Add the main thread here, and set
	it running and executing.

---
 gdb/fork-child.c |    6 ++++++
 gdb/linux-nat.c  |   11 +++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2008-08-18 23:24:09.000000000 +0100
+++ src/gdb/linux-nat.c	2008-08-18 23:36:32.000000000 +0100
@@ -2722,14 +2722,13 @@ linux_nat_wait (ptid_t ptid, struct targ
     {
       gdb_assert (!is_lwp (inferior_ptid));
 
-      inferior_ptid = BUILD_LWP (GET_PID (inferior_ptid),
-				 GET_PID (inferior_ptid));
+      /* Upgrade the main thread's ptid.  */
+      thread_change_ptid (inferior_ptid,
+			  BUILD_LWP (GET_PID (inferior_ptid),
+				     GET_PID (inferior_ptid)));
+
       lp = add_lwp (inferior_ptid);
       lp->resumed = 1;
-      /* Add the main thread to GDB's thread list.  */
-      add_thread_silent (lp->ptid);
-      set_running (lp->ptid, 1);
-      set_executing (lp->ptid, 1);
     }
 
   /* Block events while we're here.  */
Index: src/gdb/fork-child.c
===================================================================
--- src.orig/gdb/fork-child.c	2008-08-18 23:24:09.000000000 +0100
+++ src/gdb/fork-child.c	2008-08-18 23:36:32.000000000 +0100
@@ -397,6 +397,12 @@ fork_inferior (char *exec_file_arg, char
   /* Needed for wait_for_inferior stuff below.  */
   inferior_ptid = pid_to_ptid (pid);
 
+  /* We have something that executes now.  We'll be running through
+     the shell at this point, but the pid shouldn't change.  Targets
+     supporting MT should fill this task's ptid with more data as soon
+     as they can.  */
+  add_thread_silent (inferior_ptid);
+
   /* Now that we have a child process, make it our target, and
      initialize anything target-vector-specific that needs
      initializing.  */

Re: [1/7] Register the main thread/task in fork-child.c

[Daniel Jacobowitz]

On Mon, Aug 18, 2008 at 11:44:10PM +0100, Pedro Alves wrote:
> > Attached is what I checked in.
> 
> And this is what I haven't checked in yet.

Are you waiting for review of anything?  I might have mis-counted but
I think Mark, Ulrich, and I have gotten all seven between us.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [1/7] Register the main thread/task in fork-child.c

[Pedro Alves]

On Tuesday 19 August 2008 00:23:16, Daniel Jacobowitz wrote:
> On Mon, Aug 18, 2008 at 11:44:10PM +0100, Pedro Alves wrote:
> > > Attached is what I checked in.
> >
> > And this is what I haven't checked in yet.
>
> Are you waiting for review of anything?  I might have mis-counted but
> I think Mark, Ulrich, and I have gotten all seven between us.

AIX is a ptrace / fork-child client target.  I was going to wait
on having that fixed, as it would be affected by this series.

Alternativelly I could just go ahead and we fix AIX after the
fact instead.

-- 
Pedro Alves