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

[Magne Hov <mhov@undo.io>]

On Thu, Jun 29 2023, Lancelot SIX wrote:

Thanks for the comments, I'll address these along with any other comments that come in.

> 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.

Ah yes, I forgot to remove this condition after some refactoring — thanks!

>
> 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
>> 

-- 
Magne Hov | Software Engineer | Direct: +44 7395 395 648 | mhov@undo.io

Undo | Record. Replay. Resolve