[PATCH 0/2] Improve handling of thread numbers for reverse execution targets

[PATCH 0/2] Improve handling of thread numbers for reverse execution targets

[Magne Hov]

Hi,

This patchset improves the way thread numbers and thread-specific breakpoints
are handled for reverse execution targets:
- While navigating forwards and backwards in time threads should always be
  presented with the same thread number, regardless of whether they have
  previously been seen to exit.
- Thread-specific breakpoints must stay inserted when moving backwards in time
  even if the corresponding thread has terminated.

The builtin record targets don't support threads well, so I haven't been able to
test with them:
- target record-full does seem to record multiple threads, but it does not seem
  to present information about non-main threads at replay time.
- target record-btrace does not seem to let you view or select an exited thread,
  even after reversing past the thread exit.

GDB's test suite flagged these regressions, but they appear to be intermittent
between unpatched runs as well:
- gdb.base/step-over-syscall.exp
- gdb.threads/attach-many-short-lived-threads.exp
- gdb.threads/process-dies-while-handling-bp.exp

I have manually tested the info thread command and thread-specific breakpoints
with rr (https://rr-project.org) and UDB (https://undo.io), and the patches have
passed UDB's internal test suite.

I've already signed FSF agreement.

Magne Hov (2):
  gdb: keep record of thread numbers for reverse execution targets
  gdb: retain thread-specific breakpoints in reverse execution targets

 gdb/breakpoint.c | 18 +++++++++++++-----
 gdb/inferior.c   |  1 +
 gdb/inferior.h   |  7 +++++++
 gdb/thread.c     | 38 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 57 insertions(+), 7 deletions(-)

-- 
2.25.1

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

[Magne Hov]

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

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..31897bddbb1 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;
+	if (target_can_execute_reverse ())
+	  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

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

[Lancelot SIX]

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
> 

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

[Magne Hov]

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

[PATCH 2/2] gdb: retain thread-specific breakpoints in reverse execution targets

[Magne Hov]

Thread-specific breakpoints are currently ignored and removed (since
49fa26b0411d990d36f3f6c095d167f3d12afdf4) if the corresponding thread
has exited.  This makes sense for targets that only execute in the
forward direction because those breakpoints can never be hit again, but
for targets with reverse execution the same thread can be seen again.
---
 gdb/breakpoint.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index da6c8de9d14..9a25c5f663d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3157,10 +3157,12 @@ insert_breakpoint_locations (void)
 	continue;
 
       /* There is no point inserting thread-specific breakpoints if
-	 the thread no longer exists.  ALL_BP_LOCATIONS bp_location
-	 has BL->OWNER always non-NULL.  */
+	 the thread no longer exists, unless the target supports
+	 reverse execution.  ALL_BP_LOCATIONS bp_location has
+	 BL->OWNER always non-NULL.  */
       if (bl->owner->thread != -1
-	  && !valid_global_thread_id (bl->owner->thread))
+	  && !valid_global_thread_id (bl->owner->thread)
+	  && !target_can_execute_reverse ())
 	continue;
 
       switch_to_program_space_and_thread (bl->pspace);
@@ -3245,12 +3247,18 @@ remove_breakpoints (void)
   return val;
 }
 
-/* When a thread exits, remove breakpoints that are related to
-   that thread.  */
+/* When a thread exits, remove breakpoints that are related to that
+   thread and cannot be hit again.  */
 
 static void
 remove_threaded_breakpoints (struct thread_info *tp, int silent)
 {
+  /* Targets that support reverse execution may navigate to a point in
+     time where an exited thread reappears and where its breakpoints
+     are still relevant.  */
+  if (target_can_execute_reverse ())
+    return;
+
   for (breakpoint &b : all_breakpoints_safe ())
     {
       if (b.thread == tp->global_num && user_breakpoint_p (&b))
-- 
2.25.1

[PATCH v2 0/2] Improve handling of thread numbers for reverse execution targets

[Magne Hov]

I've addressed two comments from Lancelot.

I'm not aware of any maintainers that are directly responsible for any of the
files I've touched, so I'm not sure who's best placed to approve these changes.

Magne Hov (2):
  gdb: keep record of thread numbers for reverse execution targets
  gdb: retain thread-specific breakpoints in reverse execution targets

 gdb/breakpoint.c | 18 +++++++++++++-----
 gdb/inferior.c   |  1 +
 gdb/inferior.h   |  7 +++++++
 gdb/thread.c     | 38 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 57 insertions(+), 7 deletions(-)

-- 
2.40.1

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

[Magne Hov]

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

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

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

[Bruno Larsen]

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!

-- 
Cheers,
Bruno

> ---
>   gdb/inferior.c |  1 +
>   gdb/inferior.h |  7 +++++++
>   gdb/thread.c   | 38 ++++++++++++++++++++++++++++++++++++--
>   3 files changed, 44 insertions(+), 2 deletions(-)
>
> 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 ();

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

[Tom Tromey]

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

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

[Pedro Alves]

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.

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

[Magne Hov]

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

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

Undo | Record. Replay. Resolve

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

[Pedro Alves]

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

[PATCH v2 2/2] gdb: retain thread-specific breakpoints in reverse execution targets

[Magne Hov]

Thread-specific breakpoints are currently ignored and removed (since
49fa26b0411d990d36f3f6c095d167f3d12afdf4) if the corresponding thread
has exited.  This makes sense for targets that only execute in the
forward direction because those breakpoints can never be hit again, but
for targets with reverse execution the same thread can be seen again.
---
 gdb/breakpoint.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index da6c8de9d14..9a25c5f663d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3157,10 +3157,12 @@ insert_breakpoint_locations (void)
 	continue;
 
       /* There is no point inserting thread-specific breakpoints if
-	 the thread no longer exists.  ALL_BP_LOCATIONS bp_location
-	 has BL->OWNER always non-NULL.  */
+	 the thread no longer exists, unless the target supports
+	 reverse execution.  ALL_BP_LOCATIONS bp_location has
+	 BL->OWNER always non-NULL.  */
       if (bl->owner->thread != -1
-	  && !valid_global_thread_id (bl->owner->thread))
+	  && !valid_global_thread_id (bl->owner->thread)
+	  && !target_can_execute_reverse ())
 	continue;
 
       switch_to_program_space_and_thread (bl->pspace);
@@ -3245,12 +3247,18 @@ remove_breakpoints (void)
   return val;
 }
 
-/* When a thread exits, remove breakpoints that are related to
-   that thread.  */
+/* When a thread exits, remove breakpoints that are related to that
+   thread and cannot be hit again.  */
 
 static void
 remove_threaded_breakpoints (struct thread_info *tp, int silent)
 {
+  /* Targets that support reverse execution may navigate to a point in
+     time where an exited thread reappears and where its breakpoints
+     are still relevant.  */
+  if (target_can_execute_reverse ())
+    return;
+
   for (breakpoint &b : all_breakpoints_safe ())
     {
       if (b.thread == tp->global_num && user_breakpoint_p (&b))
-- 
2.40.1

Re: [PATCH v2 2/2] gdb: retain thread-specific breakpoints in reverse execution targets

[Bruno Larsen]

On 07/07/2023 18:24, Magne Hov via Gdb-patches wrote:
> Thread-specific breakpoints are currently ignored and removed (since
> 49fa26b0411d990d36f3f6c095d167f3d12afdf4) if the corresponding thread
> has exited.  This makes sense for targets that only execute in the
> forward direction because those breakpoints can never be hit again, but
> for targets with reverse execution the same thread can be seen again.
> ---

Hi!

Thanks for working on this. Same as the other one, it looks good to go

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

I hope a global maintainer or Markus look at it soon to approve it!

-- 
Cheers,
Bruno

>   gdb/breakpoint.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index da6c8de9d14..9a25c5f663d 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3157,10 +3157,12 @@ insert_breakpoint_locations (void)
>   	continue;
>   
>         /* There is no point inserting thread-specific breakpoints if
> -	 the thread no longer exists.  ALL_BP_LOCATIONS bp_location
> -	 has BL->OWNER always non-NULL.  */
> +	 the thread no longer exists, unless the target supports
> +	 reverse execution.  ALL_BP_LOCATIONS bp_location has
> +	 BL->OWNER always non-NULL.  */
>         if (bl->owner->thread != -1
> -	  && !valid_global_thread_id (bl->owner->thread))
> +	  && !valid_global_thread_id (bl->owner->thread)
> +	  && !target_can_execute_reverse ())
>   	continue;
>   
>         switch_to_program_space_and_thread (bl->pspace);
> @@ -3245,12 +3247,18 @@ remove_breakpoints (void)
>     return val;
>   }
>   
> -/* When a thread exits, remove breakpoints that are related to
> -   that thread.  */
> +/* When a thread exits, remove breakpoints that are related to that
> +   thread and cannot be hit again.  */
>   
>   static void
>   remove_threaded_breakpoints (struct thread_info *tp, int silent)
>   {
> +  /* Targets that support reverse execution may navigate to a point in
> +     time where an exited thread reappears and where its breakpoints
> +     are still relevant.  */
> +  if (target_can_execute_reverse ())
> +    return;
> +
>     for (breakpoint &b : all_breakpoints_safe ())
>       {
>         if (b.thread == tp->global_num && user_breakpoint_p (&b))

[PING][PATCH v2 0/2] Improve handling of thread numbers for reverse execution targets

[Magne Hov]

On Fri, Jul 07 2023, Magne Hov wrote:

> I've addressed two comments from Lancelot.
>
> I'm not aware of any maintainers that are directly responsible for any of the
> files I've touched, so I'm not sure who's best placed to approve these changes.
>
> Magne Hov (2):
>   gdb: keep record of thread numbers for reverse execution targets
>   gdb: retain thread-specific breakpoints in reverse execution targets
>
>  gdb/breakpoint.c | 18 +++++++++++++-----
>  gdb/inferior.c   |  1 +
>  gdb/inferior.h   |  7 +++++++
>  gdb/thread.c     | 38 ++++++++++++++++++++++++++++++++++++--
>  4 files changed, 57 insertions(+), 7 deletions(-)
>
> -- 
> 2.40.1
>

Ping

This has had some review from Bruno and Lancelot, but still needs review and approval from a global maintainer.

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

Undo | Record. Replay. Resolve

Re: [PING][PATCH v2 0/2] Improve handling of thread numbers for reverse execution targets

[Magne Hov]

On Fri, Aug 18 2023, Magne Hov wrote:

> On Fri, Jul 07 2023, Magne Hov wrote:
>
>> I've addressed two comments from Lancelot.
>>
>> I'm not aware of any maintainers that are directly responsible for any of the
>> files I've touched, so I'm not sure who's best placed to approve these changes.
>>
>> Magne Hov (2):
>>   gdb: keep record of thread numbers for reverse execution targets
>>   gdb: retain thread-specific breakpoints in reverse execution targets
>>
>>  gdb/breakpoint.c | 18 +++++++++++++-----
>>  gdb/inferior.c   |  1 +
>>  gdb/inferior.h   |  7 +++++++
>>  gdb/thread.c     | 38 ++++++++++++++++++++++++++++++++++++--
>>  4 files changed, 57 insertions(+), 7 deletions(-)
>>
>> -- 
>> 2.40.1
>>
>
> Ping
>
> This has had some review from Bruno and Lancelot, but still needs review and approval from a global maintainer.
>
> -- 
> Magne Hov | Software Engineer | Direct: +44 7395 395 648 | mhov@undo.io
>
> Undo | Record. Replay. Resolve

Ping

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

Undo | Record. Replay. Resolve

Re: [PATCH v2 0/2] Improve handling of thread numbers for reverse execution targets

[Tom Tromey]

>>>>> "Magne" == Magne Hov via Gdb-patches <gdb-patches@sourceware.org> writes:

Magne> I've addressed two comments from Lancelot.
Magne> I'm not aware of any maintainers that are directly responsible for any of the
Magne> files I've touched, so I'm not sure who's best placed to approve these changes.

Thank you for the patches.  I'm sorry about the delay on this, TBH I was
hoping someone else would get to it first.

I sent one comment about patch #1, but you don't need to resubmit the
series -- you can just fix that, rebuild, and check it in.

Approved-By: Tom Tromey <tom@tromey.com>

Tom