diff mbox series

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

Message ID 20230707162451.3605544-2-mhov@undo.io
State New
Headers
Series Improve handling of thread numbers for reverse execution targets |

Commit Message

Magne Hov July 7, 2023, 4:24 p.m. UTC 
  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
---
 gdb/inferior.c |  1 +
 gdb/inferior.h |  7 +++++++
 gdb/thread.c   | 38 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 44 insertions(+), 2 deletions(-)

Comments

Guinevere Larsen July 13, 2023, 12:21 p.m. UTC | #1 
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!
Tom Tromey Sept. 19, 2023, 4:33 p.m. UTC | #2 
>>>>> "Magne" == Magne Hov via Gdb-patches <gdb-patches@sourceware.org> writes:

Magne> Targets that support reverse execution may report threads that have
Magne> previously been seen to exit.  Currently, GDB assigns a new thread
Magne> number every time it sees the same thread (re)appearing, but this is
Magne> problematic because it makes the output of `info threads` inconsistent,
Magne> and because thread-specific breakpoints depend on stable thread-numbers
Magne> in order to stop on the right thread.

Thank you for the patch.

Magne> +  std::unordered_map<ptid_t, std::pair<int, int>, hash_ptid> ptid_thread_num_map;

You'll have to update this for the hash_ptid -> std::hash<ptid_t>
change.  However, that's trivial.

Tom
Pedro Alves Sept. 20, 2023, 4:42 p.m. UTC | #3 
On 2023-09-19 17:33, Tom Tromey wrote:
>>>>>> "Magne" == Magne Hov via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Magne> Targets that support reverse execution may report threads that have
> Magne> previously been seen to exit.  Currently, GDB assigns a new thread
> Magne> number every time it sees the same thread (re)appearing, but this is
> Magne> problematic because it makes the output of `info threads` inconsistent,
> Magne> and because thread-specific breakpoints depend on stable thread-numbers
> Magne> in order to stop on the right thread.
> 
> Thank you for the patch.
> 
> Magne> +  std::unordered_map<ptid_t, std::pair<int, int>, hash_ptid> ptid_thread_num_map;
> 
> You'll have to update this for the hash_ptid -> std::hash<ptid_t>
> change.  However, that's trivial.

I think I have some concerns.  Could you hold off a bit?

Sorry I've not managed to stay on top of patch as I'd wish I would.
Magne Hov Sept. 20, 2023, 5 p.m. UTC | #4 
On Wed, Sep 20 2023, Pedro Alves wrote:

> On 2023-09-19 17:33, Tom Tromey wrote:
>>>>>>> "Magne" == Magne Hov via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>> Magne> Targets that support reverse execution may report threads that have
>> Magne> previously been seen to exit.  Currently, GDB assigns a new thread
>> Magne> number every time it sees the same thread (re)appearing, but this is
>> Magne> problematic because it makes the output of `info threads` inconsistent,
>> Magne> and because thread-specific breakpoints depend on stable thread-numbers
>> Magne> in order to stop on the right thread.
>> 
>> Thank you for the patch.
>> 
>> Magne> +  std::unordered_map<ptid_t, std::pair<int, int>, hash_ptid> ptid_thread_num_map;
>> 
>> You'll have to update this for the hash_ptid -> std::hash<ptid_t>
>> change.  However, that's trivial.
>
> I think I have some concerns.  Could you hold off a bit?

No worries, I'll hold off until you've had time to have a look.

>
> Sorry I've not managed to stay on top of patch as I'd wish I would.
>
Pedro Alves Sept. 20, 2023, 5:13 p.m. UTC | #5 
On 2023-07-07 17:24, Magne Hov via Gdb-patches wrote:

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

What if the exited thread that we are re-adding is the current thread?
delete_exited_threads won't delete it, because you can't delete the
current thread.  (See thread_info::deletable().)

> +
>    /* 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.  */

I'm worried a little about the assumption that only targets
that don't support reverse execution need to handle the case
of thread identifiers being recycled.  If you record gdb.threads/tid-reuse.exp
you should hit the tid reuse case, for instance.
Wouldn't it be better to distinguish adding a thread when executing forward
(in which case we treat the thread id as potentially recycled by the OS), vs
executing backward or replaying forward (in which case we reuse the gdb
thread id from the previous time.)

Pedro Alves
diff mbox series

Patch

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