Re: RFC: Re: [patch] Fix for 'info threads' crashes if zombie threads exist

[Daniel Jacobowitz <drow@false.org>]

On Tue, Jun 20, 2006 at 03:07:40PM -0400, Daniel Jacobowitz wrote:
> I think the real fix to this problem is going to involve less
> dependence on thread IDs.  I've been migrating the code away from that
> and I'll try to find some time in the next week to finish the job;
> maybe that will help.

Here's an alternative patch that seems to work for the same test.
Could one of you let me know if it also helps for the problems you saw?

The main change is to remove the thread_db_thread_alive function.  My
ptid_t representation change means that we can call the linux-nat.c
implementation directly.  This change means we might have two threads
"live" at the same time with the same TID - but they'll have different
LWP IDs, so different PTIDs, so GDB won't get confused.

It also (long overdue) removes the dependence on fill_gregset, and
removes a not especially useful call into libthread_db for converting
threads to strings.  There are a number of more possible cleanups,
but this hits the big ones.

-- 
Daniel Jacobowitz
CodeSourcery

2006-07-12  Daniel Jacobowitz  <dan@codesourcery.com>

	* linux-thread-db.c (td_thr_getfpregs_p, td_thr_getgregs_p)
	(td_thr_setfpregs_p, td_thr_setgregs_p, thread_db_get_info)
	(thread_db_fetch_registers, thread_db_store_registers)
	(thread_db_thread_alive): Delete.
	(thread_db_load): Don't look up regset functions.
	(thread_db_pid_to_str): Simplify.
	(init_thread_db_ops): Do not set to_fetch_registers,
	to_store_registers, or to_thread_alive.

Index: linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.16
diff -u -p -r1.16 linux-thread-db.c
--- linux-thread-db.c	5 May 2006 22:42:43 -0000	1.16
+++ linux-thread-db.c	13 Jul 2006 03:53:34 -0000
@@ -105,14 +105,6 @@ static td_err_e (*td_ta_event_getmsg_p) 
 static td_err_e (*td_thr_validate_p) (const td_thrhandle_t *th);
 static td_err_e (*td_thr_get_info_p) (const td_thrhandle_t *th,
 				      td_thrinfo_t *infop);
-static td_err_e (*td_thr_getfpregs_p) (const td_thrhandle_t *th,
-				       gdb_prfpregset_t *regset);
-static td_err_e (*td_thr_getgregs_p) (const td_thrhandle_t *th,
-				      prgregset_t gregs);
-static td_err_e (*td_thr_setfpregs_p) (const td_thrhandle_t *th,
-				       const gdb_prfpregset_t *fpregs);
-static td_err_e (*td_thr_setgregs_p) (const td_thrhandle_t *th,
-				      prgregset_t gregs);
 static td_err_e (*td_thr_event_enable_p) (const td_thrhandle_t *th,
 					  int event);
 
@@ -330,27 +322,6 @@ thread_db_map_id2thr (struct thread_info
   else
     thread_info->private->th_valid = 1;
 }
-
-static td_thrinfo_t *
-thread_db_get_info (struct thread_info *thread_info)
-{
-  td_err_e err;
-
-  if (thread_info->private->ti_valid)
-    return &thread_info->private->ti;
-
-  if (!thread_info->private->th_valid)
-    thread_db_map_id2thr (thread_info, 1);
-
-  err =
-    td_thr_get_info_p (&thread_info->private->th, &thread_info->private->ti);
-  if (err != TD_OK)
-    error (_("thread_db_get_info: cannot get thread info: %s"),
-	   thread_db_err_str (err));
-
-  thread_info->private->ti_valid = 1;
-  return &thread_info->private->ti;
-}
 \f
 /* Convert between user-level thread ids and LWP ids.  */
 
@@ -461,22 +432,6 @@ thread_db_load (void)
   if (td_thr_get_info_p == NULL)
     return 0;
 
-  td_thr_getfpregs_p = verbose_dlsym (handle, "td_thr_getfpregs");
-  if (td_thr_getfpregs_p == NULL)
-    return 0;
-
-  td_thr_getgregs_p = verbose_dlsym (handle, "td_thr_getgregs");
-  if (td_thr_getgregs_p == NULL)
-    return 0;
-
-  td_thr_setfpregs_p = verbose_dlsym (handle, "td_thr_setfpregs");
-  if (td_thr_setfpregs_p == NULL)
-    return 0;
-
-  td_thr_setgregs_p = verbose_dlsym (handle, "td_thr_setgregs");
-  if (td_thr_setgregs_p == NULL)
-    return 0;
-
   /* Initialize the library.  */
   err = td_init_p ();
   if (err != TD_OK)
@@ -991,81 +946,6 @@ thread_db_xfer_partial (struct target_op
 }
 
 static void
-thread_db_fetch_registers (int regno)
-{
-  struct thread_info *thread_info;
-  prgregset_t gregset;
-  gdb_prfpregset_t fpregset;
-  td_err_e err;
-
-  if (!is_thread (inferior_ptid))
-    {
-      /* Pass the request to the target beneath us.  */
-      target_beneath->to_fetch_registers (regno);
-      return;
-    }
-
-  thread_info = find_thread_pid (inferior_ptid);
-  thread_db_map_id2thr (thread_info, 1);
-
-  err = td_thr_getgregs_p (&thread_info->private->th, gregset);
-  if (err != TD_OK)
-    error (_("Cannot fetch general-purpose registers for thread %ld: %s"),
-	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
-
-  err = td_thr_getfpregs_p (&thread_info->private->th, &fpregset);
-  if (err != TD_OK)
-    error (_("Cannot get floating-point registers for thread %ld: %s"),
-	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
-
-  /* Note that we must call supply_gregset after calling the thread_db
-     routines because the thread_db routines call ps_lgetgregs and
-     friends which clobber GDB's register cache.  */
-  supply_gregset ((gdb_gregset_t *) gregset);
-  supply_fpregset (&fpregset);
-}
-
-static void
-thread_db_store_registers (int regno)
-{
-  prgregset_t gregset;
-  gdb_prfpregset_t fpregset;
-  td_err_e err;
-  struct thread_info *thread_info;
-
-  if (!is_thread (inferior_ptid))
-    {
-      /* Pass the request to the target beneath us.  */
-      target_beneath->to_store_registers (regno);
-      return;
-    }
-
-  thread_info = find_thread_pid (inferior_ptid);
-  thread_db_map_id2thr (thread_info, 1);
-
-  if (regno != -1)
-    {
-      gdb_byte raw[MAX_REGISTER_SIZE];
-
-      regcache_raw_collect (current_regcache, regno, raw);
-      thread_db_fetch_registers (-1);
-      regcache_raw_supply (current_regcache, regno, raw);
-    }
-
-  fill_gregset ((gdb_gregset_t *) gregset, -1);
-  fill_fpregset (&fpregset, -1);
-
-  err = td_thr_setgregs_p (&thread_info->private->th, gregset);
-  if (err != TD_OK)
-    error (_("Cannot store general-purpose registers for thread %ld: %s"),
-	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
-  err = td_thr_setfpregs_p (&thread_info->private->th, &fpregset);
-  if (err != TD_OK)
-    error (_("Cannot store floating-point registers  for thread %ld: %s"),
-	   (long) GET_THREAD (inferior_ptid), thread_db_err_str (err));
-}
-
-static void
 thread_db_kill (void)
 {
   /* There's no need to save & restore inferior_ptid here, since the
@@ -1117,48 +997,6 @@ thread_db_mourn_inferior (void)
 }
 
 static int
-thread_db_thread_alive (ptid_t ptid)
-{
-  td_thrhandle_t th;
-  td_err_e err;
-
-  if (is_thread (ptid))
-    {
-      struct thread_info *thread_info;
-      thread_info = find_thread_pid (ptid);
-
-      thread_db_map_id2thr (thread_info, 0);
-      if (!thread_info->private->th_valid)
-	return 0;
-
-      err = td_thr_validate_p (&thread_info->private->th);
-      if (err != TD_OK)
-	return 0;
-
-      if (!thread_info->private->ti_valid)
-	{
-	  err =
-	    td_thr_get_info_p (&thread_info->private->th,
-			       &thread_info->private->ti);
-	  if (err != TD_OK)
-	    return 0;
-	  thread_info->private->ti_valid = 1;
-	}
-
-      if (thread_info->private->ti.ti_state == TD_THR_UNKNOWN
-	  || thread_info->private->ti.ti_state == TD_THR_ZOMBIE)
-	return 0;		/* A zombie thread.  */
-
-      return 1;
-    }
-
-  if (target_beneath->to_thread_alive)
-    return target_beneath->to_thread_alive (ptid);
-
-  return 0;
-}
-
-static int
 find_new_threads_callback (const td_thrhandle_t *th_p, void *data)
 {
   td_thrinfo_t ti;
@@ -1200,32 +1038,18 @@ thread_db_pid_to_str (ptid_t ptid)
   if (is_thread (ptid))
     {
       static char buf[64];
-      td_thrinfo_t *ti_p;
-      td_err_e err;
       struct thread_info *thread_info;
 
       thread_info = find_thread_pid (ptid);
-      thread_db_map_id2thr (thread_info, 0);
-      if (!thread_info->private->th_valid)
-	{
-	  snprintf (buf, sizeof (buf), "Thread %ld (Missing)",
-		    GET_THREAD (ptid));
-	  return buf;
-	}
-
-      ti_p = thread_db_get_info (thread_info);
-
-      if (ti_p->ti_state == TD_THR_ACTIVE && ti_p->ti_lid != 0)
-	{
-	  snprintf (buf, sizeof (buf), "Thread %ld (LWP %d)",
-		    (long) ti_p->ti_tid, ti_p->ti_lid);
-	}
+      if (thread_info == NULL)
+	snprintf (buf, sizeof (buf), "Thread %ld (LWP %ld) (Missing)",
+		  GET_THREAD (ptid), GET_LWP (ptid));
+      else if (thread_info->private->dying)
+	snprintf (buf, sizeof (buf), "Thread %ld (LWP %ld) (Exiting)",
+		  GET_THREAD (ptid), GET_LWP (ptid));
       else
-	{
-	  snprintf (buf, sizeof (buf), "Thread %ld (%s)",
-		    (long) ti_p->ti_tid,
-		    thread_db_state_str (ti_p->ti_state));
-	}
+	snprintf (buf, sizeof (buf), "Thread %ld (LWP %ld)",
+		  GET_THREAD (ptid), GET_LWP (ptid));
 
       return buf;
     }
@@ -1306,14 +1130,11 @@ init_thread_db_ops (void)
   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_fetch_registers = thread_db_fetch_registers;
-  thread_db_ops.to_store_registers = thread_db_store_registers;
   thread_db_ops.to_xfer_partial = thread_db_xfer_partial;
   thread_db_ops.to_kill = thread_db_kill;
   thread_db_ops.to_create_inferior = thread_db_create_inferior;
   thread_db_ops.to_post_startup_inferior = thread_db_post_startup_inferior;
   thread_db_ops.to_mourn_inferior = thread_db_mourn_inferior;
-  thread_db_ops.to_thread_alive = thread_db_thread_alive;
   thread_db_ops.to_find_new_threads = thread_db_find_new_threads;
   thread_db_ops.to_pid_to_str = thread_db_pid_to_str;
   thread_db_ops.to_stratum = thread_stratum;