Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8

  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.
  

On 10/19/2014 11:56 PM, Doug Evans wrote:

> 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 ();

IMO this function name is clear enough that we wouldn't
need to add that same comment to all callers.

> +
>        /* 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).

Not sure I understand this detach reference.  Are you seeing exited threads
after a detach from the current inferior?  What are the steps you used?
detach_inferior and exit_inferior are called with inferior_ptid
pointing at null_ptid so that thread can be deleted.

> +     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);

Hmm, this doesn't make that much sense to me.  Either we have an
empty list, or we can't reset the thread numbers.  Otherwise, the
next thread we add might reuse the GDB ID of one of those
exited threads, if it also reuses the target side ID (which
can easily happen when the target fakes thread ids, e.g.,
remote.c:magic_null_ptid).

Thinking about this further, I think the cases that may legitimately leave
an exited thread in the list are related to thread's reference count
having been incremented, either because a make_cleanup_restore_current_thread
cleanup is in the cleanup chain, or the user did "thread apply all
some-command-that-wipes-threads" -- detach/kill/etc., but also could
also be the remote connection closing abruptly while running any command.

do_restore_current_thread_cleanup won't restore the dead thread
as selected thread, but I think the thread will still be
in the list.

This then suggest to me that what we need is a function that
tries to garbage collect exited threads, and then call
that without checking have_inferiors() at specific points
We could start with the current places we call init_thread_list,
and look for somewhere more central in a following step.
prune_threads is close, but I don't think there's need to
query anything off of the target.

Resetting the thread numbering would still have to only be
done under have_inferiors(), or, perhaps even clearer,
only if the thread list is empty after the purging.
IOW, say that the thread numbering always starts at 1 if we're
adding a thread to an empty thread list.  With this in mind, it
might be simpler to instead reset to the thread numbering back to
1 in new_thread.

WDYT?

Thanks,
Pedro Alves