Re: [linux] fix stepping over fork in follow-child mode.

[Daniel Jacobowitz <drow at false dot org>]

On Tue, Mar 18, 2008 at 08:46:46PM +0000, Pedro Alves wrote:
> Hi,
> 
> I noticed that issuing a next over a fork call, with
> "follow-fork-mode child", wasn't working.  The problem is that when we
> update the breakpoints in the child to attach them to the current thread
> if their thread their currently attached to doesn't exist anymore, 
> inferior_ptid doesn't hold the tid yet.

Here's an alternative fix.  Vladimir, this is also the patch I was
talking about earlier on IRC.  Not tested or finished yet.

The basic idea: we only use tids for two things.  We display them to
the user and we pass them to libthread_db.  So the problematic
Linux behavior in which the tid is not available immediately for
the first thread (not until libpthread.so has initialized, to be
precise) is not a problem if we remove the tid from the ptid_t.
Instead, Linux now always puts zero there.  This lets us delete
a lot of code that was already more or less dead, save some
operations, et cetera.

NULL thread_info->private should never happen when libpthread.so's
pthread_create is used to create new threads.  That's where most of
the FIXMEs are for this patch; they're in places where if linux-nat.c
detects a newly cloned process, we don't have thread_info->private
filled in.  I think the right thing to do is to fill it in lazily
by calling td_ta_map_lwp2thr et cetera, and to handle it being NULL
for clones the thread manager doesn't know about.

I can work on this some more tomorrow, but you wanted a preview :-) 

-- 
Daniel Jacobowitz
CodeSourcery

Index: gdbthread.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbthread.h,v
retrieving revision 1.20
diff -u -p -r1.20 gdbthread.h
--- gdbthread.h	15 Mar 2008 13:53:25 -0000	1.20
+++ gdbthread.h	18 Mar 2008 23:41:17 -0000
@@ -81,6 +81,10 @@ extern struct thread_info *add_thread (p
    about new thread.  */
 extern struct thread_info *add_thread_silent (ptid_t ptid);
 
+/* Same as add_thread, and sets the private info.  */
+extern struct thread_info *add_thread_with_info (ptid_t ptid,
+						 struct private_thread_info *);
+
 /* Delete an existing thread list entry.  */
 extern void delete_thread (ptid_t);
 
Index: linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.37
diff -u -p -r1.37 linux-thread-db.c
--- linux-thread-db.c	23 Jan 2008 11:26:28 -0000	1.37
+++ linux-thread-db.c	18 Mar 2008 23:41:18 -0000
@@ -126,10 +126,6 @@ static void detach_thread (ptid_t ptid, 
 
 #define GET_PID(ptid)		ptid_get_pid (ptid)
 #define GET_LWP(ptid)		ptid_get_lwp (ptid)
-#define GET_THREAD(ptid)	ptid_get_tid (ptid)
-
-#define is_lwp(ptid)		(GET_LWP (ptid) != 0)
-#define is_thread(ptid)		(GET_THREAD (ptid) != 0)
 
 #define BUILD_LWP(lwp, pid)	ptid_build (pid, lwp, 0)
 
@@ -143,11 +139,8 @@ struct private_thread_info
   unsigned int dying:1;
 
   /* Cached thread state.  */
-  unsigned int th_valid:1;
-  unsigned int ti_valid:1;
-
   td_thrhandle_t th;
-  td_thrinfo_t ti;
+  thread_t tid;
 };
 
 
@@ -257,7 +250,7 @@ thread_get_info_callback (const td_thrha
 	   thread_db_err_str (err));
 
   /* Fill the cache.  */
-  thread_ptid = ptid_build (GET_PID (inferior_ptid), ti.ti_lid, ti.ti_tid);
+  thread_ptid = ptid_build (GET_PID (inferior_ptid), ti.ti_lid, 0);
   thread_info = find_thread_pid (thread_ptid);
 
   /* In the case of a zombie thread, don't continue.  We don't want to
@@ -266,13 +259,6 @@ thread_get_info_callback (const td_thrha
     {
       if (infop != NULL)
         *(struct thread_info **) infop = thread_info;
-      if (thread_info != NULL)
-	{
-	  memcpy (&thread_info->private->th, thp, sizeof (*thp));
-	  thread_info->private->th_valid = 1;
-	  memcpy (&thread_info->private->ti, &ti, sizeof (ti));
-	  thread_info->private->ti_valid = 1;
-	}
       return TD_THR_ZOMBIE;
     }
 
@@ -284,39 +270,11 @@ thread_get_info_callback (const td_thrha
       gdb_assert (thread_info != NULL);
     }
 
-  memcpy (&thread_info->private->th, thp, sizeof (*thp));
-  thread_info->private->th_valid = 1;
-  memcpy (&thread_info->private->ti, &ti, sizeof (ti));
-  thread_info->private->ti_valid = 1;
-
   if (infop != NULL)
     *(struct thread_info **) infop = thread_info;
 
   return 0;
 }
-
-/* Accessor functions for the thread_db information, with caching.  */
-
-static void
-thread_db_map_id2thr (struct thread_info *thread_info, int fatal)
-{
-  td_err_e err;
-
-  if (thread_info->private->th_valid)
-    return;
-
-  err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (thread_info->ptid),
-			    &thread_info->private->th);
-  if (err != TD_OK)
-    {
-      if (fatal)
-	error (_("Cannot find thread %ld: %s"),
-	       (long) GET_THREAD (thread_info->ptid),
-	       thread_db_err_str (err));
-    }
-  else
-    thread_info->private->th_valid = 1;
-}
 
 /* Convert between user-level thread ids and LWP ids.  */
 
@@ -328,11 +286,11 @@ thread_from_lwp (ptid_t ptid)
   struct thread_info *thread_info;
   ptid_t thread_ptid;
 
+  /* FIXME: Should this ever even happen?  I don't think it
+     should reach here.  */
   if (GET_LWP (ptid) == 0)
     ptid = BUILD_LWP (GET_PID (ptid), GET_PID (ptid));
 
-  gdb_assert (is_lwp (ptid));
-
   err = td_ta_map_lwp2thr_p (thread_agent, GET_LWP (ptid), &th);
   if (err != TD_OK)
     error (_("Cannot find user-level thread for LWP %ld: %s"),
@@ -352,16 +310,8 @@ thread_from_lwp (ptid_t ptid)
       && thread_info == NULL)
     return pid_to_ptid (-1);
 
-  gdb_assert (thread_info && thread_info->private->ti_valid);
-
-  return ptid_build (GET_PID (ptid), GET_LWP (ptid),
-		     thread_info->private->ti.ti_tid);
-}
-
-static ptid_t
-lwp_from_thread (ptid_t ptid)
-{
-  return BUILD_LWP (GET_LWP (ptid), GET_PID (ptid));
+  gdb_assert (ptid_get_tid (ptid) == 0);
+  return ptid;
 }
 
 
@@ -672,6 +622,7 @@ static void
 attach_thread (ptid_t ptid, const td_thrhandle_t *th_p,
 	       const td_thrinfo_t *ti_p)
 {
+  struct private_thread_info *private;
   struct thread_info *tp;
   td_err_e err;
 
@@ -705,10 +656,21 @@ attach_thread (ptid_t ptid, const td_thr
   if (lin_lwp_attach_lwp (BUILD_LWP (ti_p->ti_lid, GET_PID (ptid))) < 0)
     return;
 
+  /* Construct the thread's private data.  */
+  private = xmalloc (sizeof (struct private_thread_info));
+  memset (private, 0, sizeof (struct private_thread_info));
+
+  /* A thread ID of zero may mean the thread library has not initialized
+     yet.  But we shouldn't even get here if that's the case.  FIXME:
+     if we change GDB to always have at least one thread in the thread
+     list this will have to go somewhere else; maybe private == NULL
+     until the thread_db target claims it.  */
+  gdb_assert (ti_p->ti_tid != 0);
+  private->th = *th_p;
+  private->tid = ti_p->ti_tid;
+
   /* Add the thread to GDB's thread list.  */
-  tp = add_thread (ptid);
-  tp->private = xmalloc (sizeof (struct private_thread_info));
-  memset (tp->private, 0, sizeof (struct private_thread_info));
+  tp = add_thread_with_info (ptid, private);
 
   /* Enable thread event reporting for this thread.  */
   err = td_thr_event_enable_p (th_p, 1);
@@ -742,47 +704,12 @@ thread_db_detach (char *args, int from_t
 {
   disable_thread_event_reporting ();
 
-  /* There's no need to save & restore inferior_ptid here, since the
-     inferior is not supposed to survive this function call.  */
-  inferior_ptid = lwp_from_thread (inferior_ptid);
-
   target_beneath->to_detach (args, from_tty);
 
   /* Should this be done by detach_command?  */
   target_mourn_inferior ();
 }
 
-static int
-clear_lwpid_callback (struct thread_info *thread, void *dummy)
-{
-  /* If we know that our thread implementation is 1-to-1, we could save
-     a certain amount of information; it's not clear how much, so we
-     are always conservative.  */
-
-  thread->private->th_valid = 0;
-  thread->private->ti_valid = 0;
-
-  return 0;
-}
-
-static void
-thread_db_resume (ptid_t ptid, int step, enum target_signal signo)
-{
-  struct cleanup *old_chain = save_inferior_ptid ();
-
-  if (GET_PID (ptid) == -1)
-    inferior_ptid = lwp_from_thread (inferior_ptid);
-  else if (is_thread (ptid))
-    ptid = lwp_from_thread (ptid);
-
-  /* Clear cached data which may not be valid after the resume.  */
-  iterate_over_threads (clear_lwpid_callback, NULL);
-
-  target_beneath->to_resume (ptid, step, signo);
-
-  do_cleanups (old_chain);
-}
-
 /* Check if PID is currently stopped at the location of a thread event
    breakpoint location.  If it is, read the event message and act upon
    the event.  */
@@ -833,7 +760,7 @@ check_event (ptid_t ptid)
       if (err != TD_OK)
 	error (_("Cannot get thread info: %s"), thread_db_err_str (err));
 
-      ptid = ptid_build (GET_PID (ptid), ti.ti_lid, ti.ti_tid);
+      ptid = ptid_build (GET_PID (ptid), ti.ti_lid, 0);
 
       switch (msg.event)
 	{
@@ -865,9 +792,6 @@ thread_db_wait (ptid_t ptid, struct targ
 {
   extern ptid_t trap_ptid;
 
-  if (GET_PID (ptid) != -1 && is_thread (ptid))
-    ptid = lwp_from_thread (ptid);
-
   ptid = target_beneath->to_wait (ptid, ourstatus);
 
   if (ourstatus->kind == TARGET_WAITKIND_EXITED
@@ -913,15 +837,6 @@ thread_db_wait (ptid_t ptid, struct targ
 }
 
 static void
-thread_db_kill (void)
-{
-  /* There's no need to save & restore inferior_ptid here, since the
-     inferior isn't supposed to survive this function call.  */
-  inferior_ptid = lwp_from_thread (inferior_ptid);
-  target_beneath->to_kill ();
-}
-
-static void
 thread_db_mourn_inferior (void)
 {
   /* Forget about the child's process ID.  We shouldn't need it
@@ -954,7 +869,7 @@ find_new_threads_callback (const td_thrh
   if (ti.ti_state == TD_THR_UNKNOWN || ti.ti_state == TD_THR_ZOMBIE)
     return 0;			/* A zombie -- ignore.  */
 
-  ptid = ptid_build (GET_PID (inferior_ptid), ti.ti_lid, ti.ti_tid);
+  ptid = ptid_build (GET_PID (inferior_ptid), ti.ti_lid, 0);
 
   if (ti.ti_tid == 0)
     {
@@ -994,18 +909,21 @@ thread_db_find_new_threads (void)
 static char *
 thread_db_pid_to_str (ptid_t ptid)
 {
-  if (is_thread (ptid))
+  struct thread_info *thread_info = find_thread_pid (ptid);
+
+  if (thread_info != NULL)
     {
       static char buf[64];
-      struct thread_info *thread_info;
+      thread_t tid;
+
+      /* FIXME: What will set ->private?  */
+      gdb_assert (thread_info->private != NULL);
+      /* FIXME: What will ensure tid_valid?  */
+      tid = thread_info->private->tid;
 
       thread_info = find_thread_pid (ptid);
-      if (thread_info == NULL)
-	snprintf (buf, sizeof (buf), "Thread 0x%lx (LWP %ld) (Missing)",
-		  GET_THREAD (ptid), GET_LWP (ptid));
-      else
-	snprintf (buf, sizeof (buf), "Thread 0x%lx (LWP %ld)",
-		  GET_THREAD (ptid), GET_LWP (ptid));
+      snprintf (buf, sizeof (buf), "Thread 0x%lx (LWP %ld)",
+		tid, GET_LWP (ptid));
 
       return buf;
     }
@@ -1028,16 +946,6 @@ thread_db_extra_thread_info (struct thre
   return NULL;
 }
 
-/* Return 1 if this thread has the same LWP as the passed PTID.  */
-
-static int
-same_ptid_callback (struct thread_info *thread, void *arg)
-{
-  ptid_t *ptid_p = arg;
-
-  return GET_LWP (thread->ptid) == GET_LWP (*ptid_p);
-}
-
 /* Get the address of the thread local variable in load module LM which
    is stored at OFFSET within the thread local storage for thread PTID.  */
 
@@ -1046,26 +954,19 @@ thread_db_get_thread_local_address (ptid
 				    CORE_ADDR lm,
 				    CORE_ADDR offset)
 {
+  struct thread_info *thread_info;
+
   /* If we have not discovered any threads yet, check now.  */
-  if (!is_thread (ptid) && !have_threads ())
+  if (!have_threads ())
     thread_db_find_new_threads ();
 
-  /* Try to find a matching thread if we still have the LWP ID instead
-     of the thread ID.  */
-  if (!is_thread (ptid))
-    {
-      struct thread_info *thread;
-
-      thread = iterate_over_threads (same_ptid_callback, &ptid);
-      if (thread != NULL)
-	ptid = thread->ptid;
-    }
+  /* Find the matching thread.  */
+  thread_info = find_thread_pid (ptid);
 
-  if (is_thread (ptid))
+  if (thread_info != NULL)
     {
       td_err_e err;
       void *address;
-      struct thread_info *thread_info;
 
       /* glibc doesn't provide the needed interface.  */
       if (!td_thr_tls_get_addr_p)
@@ -1075,10 +976,8 @@ thread_db_get_thread_local_address (ptid
       /* Caller should have verified that lm != 0.  */
       gdb_assert (lm != 0);
 
-      /* Get info about the thread.  */
-      thread_info = find_thread_pid (ptid);
-      gdb_assert (thread_info);
-      thread_db_map_id2thr (thread_info, 1);
+      /* Something should already have arranged this... FIXME: right?  */
+      gdb_assert (thread_info->private);
 
       /* Finally, get the address of the variable.  */
       err = td_thr_tls_get_addr_p (&thread_info->private->th,
@@ -1122,9 +1021,7 @@ init_thread_db_ops (void)
   thread_db_ops.to_longname = "multi-threaded child process.";
   thread_db_ops.to_doc = "Threads and pthreads support.";
   thread_db_ops.to_detach = thread_db_detach;
-  thread_db_ops.to_resume = thread_db_resume;
   thread_db_ops.to_wait = thread_db_wait;
-  thread_db_ops.to_kill = thread_db_kill;
   thread_db_ops.to_mourn_inferior = thread_db_mourn_inferior;
   thread_db_ops.to_find_new_threads = thread_db_find_new_threads;
   thread_db_ops.to_pid_to_str = thread_db_pid_to_str;
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.62
diff -u -p -r1.62 thread.c
--- thread.c	15 Mar 2008 13:53:25 -0000	1.62
+++ thread.c	18 Mar 2008 23:41:18 -0000
@@ -132,10 +132,12 @@ add_thread_silent (ptid_t ptid)
 }
 
 struct thread_info *
-add_thread (ptid_t ptid)
+add_thread_with_info (ptid_t ptid, struct private_thread_info *private)
 {
   struct thread_info *result = add_thread_silent (ptid);
 
+  result->private = private;
+
   if (print_thread_events)
     printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
 
@@ -144,6 +146,12 @@ add_thread (ptid_t ptid)
   return result;
 }
 
+struct thread_info *
+add_thread (ptid_t ptid)
+{
+  return add_thread_with_info (ptid, NULL);
+}
+
 void
 delete_thread (ptid_t ptid)
 {
Index: testsuite/gdb.threads/fork-child-threads.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/fork-child-threads.exp,v
retrieving revision 1.1
diff -u -p -r1.1 fork-child-threads.exp
--- testsuite/gdb.threads/fork-child-threads.exp	2 Jan 2008 13:36:38 -0000	1.1
+++ testsuite/gdb.threads/fork-child-threads.exp	18 Mar 2008 23:41:18 -0000
@@ -38,6 +38,10 @@ if ![runto_main] then {
 
 gdb_test "set follow-fork-mode child"
 gdb_breakpoint "start"
+
+# Make sure we can step over fork without losing our breakpoint.
+gdb_test "next" ".*pthread_create \\(&thread, NULL, start, NULL\\);.*" "next over fork"
+
 gdb_test "continue" "Breakpoint 2, start.*" "get to the spawned thread"
 
 # Wrong: