* [PATCH 0/2] Improve handling of thread numbers for reverse execution targets @ 2023-06-29 8:36 Magne Hov 2023-06-29 8:36 ` [PATCH 1/2] gdb: keep record " Magne Hov ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Magne Hov @ 2023-06-29 8:36 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] gdb: keep record of thread numbers for reverse execution targets 2023-06-29 8:36 [PATCH 0/2] Improve handling of thread numbers for reverse execution targets Magne Hov @ 2023-06-29 8:36 ` Magne Hov 2023-06-29 9:01 ` Lancelot SIX 2023-06-29 8:36 ` [PATCH 2/2] gdb: retain thread-specific breakpoints in " Magne Hov 2023-07-07 16:24 ` [PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov 2 siblings, 1 reply; 17+ messages in thread From: Magne Hov @ 2023-06-29 8:36 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] gdb: keep record of thread numbers for reverse execution targets 2023-06-29 8:36 ` [PATCH 1/2] gdb: keep record " Magne Hov @ 2023-06-29 9:01 ` Lancelot SIX 2023-06-29 9:38 ` Magne Hov 0 siblings, 1 reply; 17+ messages in thread From: Lancelot SIX @ 2023-06-29 9:01 UTC (permalink / raw) To: Magne Hov; +Cc: gdb-patches 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] gdb: keep record of thread numbers for reverse execution targets 2023-06-29 9:01 ` Lancelot SIX @ 2023-06-29 9:38 ` Magne Hov 0 siblings, 0 replies; 17+ messages in thread From: Magne Hov @ 2023-06-29 9:38 UTC (permalink / raw) To: Lancelot SIX; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] gdb: retain thread-specific breakpoints in reverse execution targets 2023-06-29 8:36 [PATCH 0/2] Improve handling of thread numbers for reverse execution targets Magne Hov 2023-06-29 8:36 ` [PATCH 1/2] gdb: keep record " Magne Hov @ 2023-06-29 8:36 ` Magne Hov 2023-07-07 16:24 ` [PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov 2 siblings, 0 replies; 17+ messages in thread From: Magne Hov @ 2023-06-29 8:36 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 0/2] Improve handling of thread numbers for reverse execution targets 2023-06-29 8:36 [PATCH 0/2] Improve handling of thread numbers for reverse execution targets Magne Hov 2023-06-29 8:36 ` [PATCH 1/2] gdb: keep record " Magne Hov 2023-06-29 8:36 ` [PATCH 2/2] gdb: retain thread-specific breakpoints in " Magne Hov @ 2023-07-07 16:24 ` Magne Hov 2023-07-07 16:24 ` [PATCH v2 1/2] gdb: keep record " Magne Hov ` (3 more replies) 2 siblings, 4 replies; 17+ messages in thread From: Magne Hov @ 2023-07-07 16:24 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] gdb: keep record of thread numbers for reverse execution targets 2023-07-07 16:24 ` [PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov @ 2023-07-07 16:24 ` Magne Hov 2023-07-13 12:21 ` Bruno Larsen ` (2 more replies) 2023-07-07 16:24 ` [PATCH v2 2/2] gdb: retain thread-specific breakpoints in " Magne Hov ` (2 subsequent siblings) 3 siblings, 3 replies; 17+ messages in thread From: Magne Hov @ 2023-07-07 16:24 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] gdb: keep record of thread numbers for reverse execution targets 2023-07-07 16:24 ` [PATCH v2 1/2] gdb: keep record " Magne Hov @ 2023-07-13 12:21 ` Bruno Larsen 2023-09-19 16:33 ` Tom Tromey 2023-09-20 17:13 ` Pedro Alves 2 siblings, 0 replies; 17+ messages in thread From: Bruno Larsen @ 2023-07-13 12:21 UTC (permalink / raw) To: Magne Hov, gdb-patches 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 (); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] gdb: keep record of thread numbers for reverse execution targets 2023-07-07 16:24 ` [PATCH v2 1/2] gdb: keep record " Magne Hov 2023-07-13 12:21 ` Bruno Larsen @ 2023-09-19 16:33 ` Tom Tromey 2023-09-20 16:42 ` Pedro Alves 2023-09-20 17:13 ` Pedro Alves 2 siblings, 1 reply; 17+ messages in thread From: Tom Tromey @ 2023-09-19 16:33 UTC (permalink / raw) To: Magne Hov via Gdb-patches; +Cc: Magne Hov >>>>> "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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] gdb: keep record of thread numbers for reverse execution targets 2023-09-19 16:33 ` Tom Tromey @ 2023-09-20 16:42 ` Pedro Alves 2023-09-20 17:00 ` Magne Hov 0 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2023-09-20 16:42 UTC (permalink / raw) To: Tom Tromey, Magne Hov via Gdb-patches 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] gdb: keep record of thread numbers for reverse execution targets 2023-09-20 16:42 ` Pedro Alves @ 2023-09-20 17:00 ` Magne Hov 0 siblings, 0 replies; 17+ messages in thread From: Magne Hov @ 2023-09-20 17:00 UTC (permalink / raw) To: Pedro Alves, Tom Tromey, Magne Hov via Gdb-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] gdb: keep record of thread numbers for reverse execution targets 2023-07-07 16:24 ` [PATCH v2 1/2] gdb: keep record " Magne Hov 2023-07-13 12:21 ` Bruno Larsen 2023-09-19 16:33 ` Tom Tromey @ 2023-09-20 17:13 ` Pedro Alves 2 siblings, 0 replies; 17+ messages in thread From: Pedro Alves @ 2023-09-20 17:13 UTC (permalink / raw) To: Magne Hov, gdb-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] gdb: retain thread-specific breakpoints in reverse execution targets 2023-07-07 16:24 ` [PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov 2023-07-07 16:24 ` [PATCH v2 1/2] gdb: keep record " Magne Hov @ 2023-07-07 16:24 ` Magne Hov 2023-07-13 12:22 ` Bruno Larsen 2023-08-18 14:27 ` [PING][PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov 2023-09-19 16:34 ` [PATCH " Tom Tromey 3 siblings, 1 reply; 17+ messages in thread From: Magne Hov @ 2023-07-07 16:24 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] gdb: retain thread-specific breakpoints in reverse execution targets 2023-07-07 16:24 ` [PATCH v2 2/2] gdb: retain thread-specific breakpoints in " Magne Hov @ 2023-07-13 12:22 ` Bruno Larsen 0 siblings, 0 replies; 17+ messages in thread From: Bruno Larsen @ 2023-07-13 12:22 UTC (permalink / raw) To: Magne Hov, gdb-patches 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)) ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PING][PATCH v2 0/2] Improve handling of thread numbers for reverse execution targets 2023-07-07 16:24 ` [PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov 2023-07-07 16:24 ` [PATCH v2 1/2] gdb: keep record " Magne Hov 2023-07-07 16:24 ` [PATCH v2 2/2] gdb: retain thread-specific breakpoints in " Magne Hov @ 2023-08-18 14:27 ` Magne Hov 2023-09-18 11:38 ` Magne Hov 2023-09-19 16:34 ` [PATCH " Tom Tromey 3 siblings, 1 reply; 17+ messages in thread From: Magne Hov @ 2023-08-18 14:27 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PING][PATCH v2 0/2] Improve handling of thread numbers for reverse execution targets 2023-08-18 14:27 ` [PING][PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov @ 2023-09-18 11:38 ` Magne Hov 0 siblings, 0 replies; 17+ messages in thread From: Magne Hov @ 2023-09-18 11:38 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] Improve handling of thread numbers for reverse execution targets 2023-07-07 16:24 ` [PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov ` (2 preceding siblings ...) 2023-08-18 14:27 ` [PING][PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov @ 2023-09-19 16:34 ` Tom Tromey 3 siblings, 0 replies; 17+ messages in thread From: Tom Tromey @ 2023-09-19 16:34 UTC (permalink / raw) To: Magne Hov via Gdb-patches; +Cc: Magne Hov >>>>> "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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-09-20 17:13 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-29 8:36 [PATCH 0/2] Improve handling of thread numbers for reverse execution targets Magne Hov 2023-06-29 8:36 ` [PATCH 1/2] gdb: keep record " Magne Hov 2023-06-29 9:01 ` Lancelot SIX 2023-06-29 9:38 ` Magne Hov 2023-06-29 8:36 ` [PATCH 2/2] gdb: retain thread-specific breakpoints in " Magne Hov 2023-07-07 16:24 ` [PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov 2023-07-07 16:24 ` [PATCH v2 1/2] gdb: keep record " Magne Hov 2023-07-13 12:21 ` Bruno Larsen 2023-09-19 16:33 ` Tom Tromey 2023-09-20 16:42 ` Pedro Alves 2023-09-20 17:00 ` Magne Hov 2023-09-20 17:13 ` Pedro Alves 2023-07-07 16:24 ` [PATCH v2 2/2] gdb: retain thread-specific breakpoints in " Magne Hov 2023-07-13 12:22 ` Bruno Larsen 2023-08-18 14:27 ` [PING][PATCH v2 0/2] Improve handling of thread numbers for " Magne Hov 2023-09-18 11:38 ` Magne Hov 2023-09-19 16:34 ` [PATCH " Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).