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

Magne Hov via Gdb-patches
2023-06-29T09:38:35Z

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