Re: [PATCH v2 1/2] gdb: keep record of thread numbers for reverse execution targets

[Pedro Alves <pedro@palves.net>]

On 2023-07-07 17:24, Magne Hov via Gdb-patches wrote:

> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -290,6 +290,15 @@ add_thread_silent (process_stratum_target *targ, ptid_t ptid)
>  			inf->num, ptid.to_string ().c_str (),
>  			targ->shortname ());
>  
> + /* Targets that support reverse execution can see the same thread
> +    being added multiple times.  If the state for an exited thread is
> +    still present in the inferior's thread list it must be cleaned up
> +    now before we add a new non-exited entry for the same thread.
> +    Targets without reverse execution are not affected by this because
> +    they do not reuse thread numbers.  */
> +  if (target_can_execute_reverse ())
> +    delete_exited_threads ();

What if the exited thread that we are re-adding is the current thread?
delete_exited_threads won't delete it, because you can't delete the
current thread.  (See thread_info::deletable().)

> +
>    /* We may have an old thread with the same id in the thread list.
>       If we do, it must be dead, otherwise we wouldn't be adding a new
>       thread with the same id.  The OS is reusing this id --- delete
> @@ -332,8 +341,33 @@ thread_info::thread_info (struct inferior *inf_, ptid_t ptid_)
>  {
>    gdb_assert (inf_ != NULL);
>  
> -  this->global_num = ++highest_thread_num;
> -  this->per_inf_num = ++inf_->highest_thread_num;
> +  /* Targets that support reverse execution may see the same thread be
> +     created multiple times so a historical record must be maintained
> +     and queried.  For targets without reverse execution we don't look
> +     up historical thread numbers because it leaves us vulnerable to
> +     collisions between thread identifiers that have been recycled by
> +     the target operating system.  */

I'm worried a little about the assumption that only targets
that don't support reverse execution need to handle the case
of thread identifiers being recycled.  If you record gdb.threads/tid-reuse.exp
you should hit the tid reuse case, for instance.
Wouldn't it be better to distinguish adding a thread when executing forward
(in which case we treat the thread id as potentially recycled by the OS), vs
executing backward or replaying forward (in which case we reuse the gdb
thread id from the previous time.)

Pedro Alves