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

[Lancelot SIX <lsix@lancelotsix.com>]

Hi Magne,

I have some comments below.

> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index caa8e4d494a..33eb857645e 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -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 {

The '{' should be on the next line.

> +	this->global_num = ++highest_thread_num;
> +	this->per_inf_num = ++inf_->highest_thread_num;
> +	if (target_can_execute_reverse ())

This code is already in a block guarded by target_can_execute_reverse (),
so doing this check again seems redundant.  I think inserting to the map
can be done unconditionally here.

Best,
Lancelot.

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