Re: Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8

[Doug Evans <xdje42@gmail.com>]

Pedro Alves <palves@redhat.com> writes:
>> The most succinct way of expressing what I'm looking for that I can
>> think of is that I want to understand this code, and I don't.
>> There are several instances of the !have_inferiors() check, I picked
>> this one because it seemed like as good a choice as any.
>
> init_thread_list does two things:
>
>  - wipes all threads.
>  - resets the thread id counter back to 0, so the next
>    thread is gdb thread number 1.
>
> The main point of calling init_thread_list if there are
> no live inferiors, is to reset the thread id counter.
> That is so that with "run; kill; run", you end up with
> the main thread being gdb thread number 1, in both
> runs.

That part I can understand.

> Now, if there are other live inferiors, then we shouldn't wipe
> their threads.  And as gdb thread ids are global currently,
> not private per-inferior/process, then we obviously shouldn't
> reset the  thread id counter either.

This I can grok too.

However, what if some code is not properly clearing a thread
from the list? (which may involve just tagging it as exited and leaving
gc'ing it until later)  Such a bug will be hidden in the
!have_inferiors() case because we take a sledgehammer to the list.
To the reader it's not clear whether wiping the list is necessary,
or just follows along for the ride, so to speak, and is essentially
a no-op, because we're using init_thread_list to reset thread numbering.

I would expect the attached patch to be a no-op.
I don't have an opinion on committing it.
I do have an opinion that the current code is confusing,
and this is one way to help clear it up.

2014-10-19  Doug Evans  <xdje42@gmail.com>

	* thread.c (reset_thread_numbering): New function.
	(init_thread_list): Call it.
	* gdbthread.h (reset_thread_numbering): Declare.
	* fork-child.c (fork_inferior): Call reset_thread_numbering instead of
	init_thread_list.
	* infcmd.c (kill_command): Ditto.
	(detach_command): Ditto.
	* remote.c (extended_remote_create_inferior): Ditto.

diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 4c316b1..149eb22 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -379,7 +379,10 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
   environ = save_our_env;
 
   if (!have_inferiors ())
-    init_thread_list ();
+    {
+      /* Reset thread numbering to begin back at 1.  */
+      reset_thread_numbering ();
+    }
 
   inf = current_inferior ();
 
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index fb47bae..4b3bf7f 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -261,6 +261,11 @@ struct thread_info
   struct btrace_thread_info btrace;
 };
 
+/* Reset thread numbering so that they begin at 1 again.
+   This can only be called when it is known that there are no current
+   non-exited threads.  A typical test is !have_inferiors().  */
+extern void reset_thread_numbering (void);
+
 /* Create an empty thread list, or empty the existing one.  */
 extern void init_thread_list (void);
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 4415b31..0d06996 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2399,7 +2399,8 @@ kill_command (char *arg, int from_tty)
      with their threads.  */
   if (!have_inferiors ())
     {
-      init_thread_list ();		/* Destroy thread info.  */
+      /* Reset thread numbering to begin back at 1.  */
+      reset_thread_numbering ();
 
       /* Killing off the inferior can leave us with a core file.  If
 	 so, print the state we are left in.  */
@@ -2787,10 +2788,11 @@ detach_command (char *args, int from_tty)
   if (!gdbarch_has_global_solist (target_gdbarch ()))
     no_shared_libraries (NULL, from_tty);
 
-  /* If we still have inferiors to debug, then don't mess with their
-     threads.  */
   if (!have_inferiors ())
-    init_thread_list ();
+    {
+      /* No more inferiors, reset thread numbering back to 1.  */
+      reset_thread_numbering ();
+    }
 
   if (deprecated_detach_hook)
     deprecated_detach_hook ();
diff --git a/gdb/remote.c b/gdb/remote.c
index 20f2988..6c43da8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8035,10 +8035,12 @@ extended_remote_create_inferior (struct target_ops *ops,
 
   if (!have_inferiors ())
     {
+      /* Reset thread numbering to begin back at 1.  */
+      reset_thread_numbering ();
+
       /* Clean up from the last time we ran, before we mark the target
 	 running again.  This will mark breakpoints uninserted, and
 	 get_offsets may insert breakpoints.  */
-      init_thread_list ();
       init_wait_for_inferior ();
     }
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 5c94264..789ab7e 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -195,15 +195,30 @@ free_thread (struct thread_info *tp)
   xfree (tp);
 }
 
+/* See gdbthread.h.  */
+
 void
-init_thread_list (void)
+reset_thread_numbering (void)
 {
-  struct thread_info *tp, *tpnext;
+  struct thread_info *tp;
+
+  /* While IWBN to assert thread_list is NULL, there could still be
+     threads on it (e.g., if you detach from the current inferior).
+     Instead verify all threads on the list have exited.  They will be
+     garbage-collected as we're able to.  */
+  for (tp = thread_list; tp != NULL; tp = tp->next)
+    gdb_assert (tp->state == THREAD_EXITED);
 
   highest_thread_num = 0;
 
-  if (!thread_list)
-    return;
+  /* Take the opportunity to update this since we know it's zero.  */
+  threads_executing = 0;
+}
+
+void
+init_thread_list (void)
+{
+  struct thread_info *tp, *tpnext;
 
   for (tp = thread_list; tp; tp = tpnext)
     {
@@ -212,7 +227,8 @@ init_thread_list (void)
     }
 
   thread_list = NULL;
-  threads_executing = 0;
+
+  reset_thread_numbering ();
 }
 
 /* Allocate a new thread with target id PTID and add it to the thread