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

[Bruno Larsen <blarsen@redhat.com>]

On 07/07/2023 18:24, Magne Hov via Gdb-patches wrote:
> Targets that support reverse execution may report threads that have
> previously been seen to exit.  Currently, GDB assigns a new thread
> number every time it sees the same thread (re)appearing, but this is
> problematic because it makes the output of `info threads` inconsistent,
> and because thread-specific breakpoints depend on stable thread-numbers
> in order to stop on the right thread.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23454

Hi!

Thanks for working on this. This looks like a good addition, and 
introduces no regressions.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

I hope a global maintainers or Markus look at it soon!

-- 
Cheers,
Bruno

> ---
>   gdb/inferior.c |  1 +
>   gdb/inferior.h |  7 +++++++
>   gdb/thread.c   | 38 ++++++++++++++++++++++++++++++++++++--
>   3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index eee4785fbf7..b6323110e69 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -258,6 +258,7 @@ inferior::clear_thread_list (bool silent)
>   	delete thr;
>       });
>     ptid_thread_map.clear ();
> +  ptid_thread_num_map.clear ();
>   }
>   
>   /* Notify interpreters and observers that inferior INF was removed.  */
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index caa8e4d494a..33eb857645e 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -569,6 +569,13 @@ class inferior : public refcounted_object,
>     /* The highest thread number this inferior ever had.  */
>     int highest_thread_num = 0;
>   
> +  /* A map of ptid_t to global thread number and per-inferior thread
> +     number, used for targets that support reverse execution.  These
> +     mappings are maintained for the lifetime of the inferior so that
> +     thread numbers can be reused for threads that reappear after
> +     having exited.  */
> +  std::unordered_map<ptid_t, std::pair<int, int>, hash_ptid> ptid_thread_num_map;
> +
>     /* State of GDB control of inferior process execution.
>        See `struct inferior_control_state'.  */
>     inferior_control_state control;
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 7f7f035b5ab..ef3da68fc30 100644
> --- 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 ();
> +
>     /* 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.  */
> +  if (target_can_execute_reverse ())
> +    {
> +      auto pair = inf_->ptid_thread_num_map.find (ptid_);
> +      if (pair != inf_->ptid_thread_num_map.end ())
> +	{
> +	  this->global_num = pair->second.first;
> +	  this->per_inf_num = pair->second.second;
> +	}
> +      else
> +	{
> +	  this->global_num = ++highest_thread_num;
> +	  this->per_inf_num = ++inf_->highest_thread_num;
> +	  inf_->ptid_thread_num_map[ptid_] = std::make_pair (this->global_num,
> +							     this->per_inf_num);
> +	}
> +    }
> +  else
> +    {
> +      this->global_num = ++highest_thread_num;
> +      this->per_inf_num = ++inf_->highest_thread_num;
> +    }
>   
>     /* Nothing to follow yet.  */
>     this->pending_follow.set_spurious ();