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

Lancelot SIX via Gdb-patches
2023-06-29T09:02:17Z

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
>