From: Pedro Alves Date: Thu, 18 Jun 2020 20:28:37 +0000 (+0100) Subject: Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR 25412) X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=3922b302645fda04da42a5279399578ae2f6206c;p=binutils-gdb.git Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR 25412) In PR 25412, Simon noticed that after the multi-target series, the tid-reuse.exp testcase manages to create a duplicate thread in the thread list. Or rather, two threads with the same PTID. add_thread_silent has code in place to detect the case of a new thread reusing some older thread's ptid, but it doesn't work correctly anymore when the old thread is NOT the current thread and it has a refcount higher than 0. Either condition prevents a thread from being deleted, but the refcount case wasn't being considered. I think the reason that case wasn't considered is that that code predates thread_info refcounting. Back when it was originally written, delete_thread always deleted the thread. That add_thread_silent code in question has some now-unnecessary warts, BTW. For instance, this: /* Make switch_to_thread not read from the thread. */ new_thr->state = THREAD_EXITED; ... used to be required because switch_to_thread would update 'stop_pc' otherwise. I.e., it would read registers from an exited thread otherwise. switch_to_thread no longer reads the stop_pc, since: commit f2ffa92bbce9dd5fbedc138ac2a3bc8a88327d09 Author: Pedro Alves AuthorDate: Thu Jun 28 20:18:24 2018 +0100 gdb: Eliminate the 'stop_pc' global Also, if the ptid of the now-gone current thread is reused, we currently return from add_thread_silent with the current thread pointing at the _new_ thread. Either pointing at the old thread, or at no thread selected would be reasonable. But pointing at an unrelated thread (the new thread that happens to reuse the ptid) is just broken. Seems like I was the one who wrote it like that but I have no clue why, FWIW. Currently, an exited thread kept in the thread list still holds its original ptid. The idea was that we need the ptid to be able to temporarily switch to another thread and then switch back to the original thread, because thread switching is really inferior_ptid switching. Switching back to the original thread requires a ptid lookup. Now, in order to avoid exited threads with the same ptid as a live thread in the same thread list, one thing I considered (and tried) was to change an exited thread's ptid to minus_one_ptid. However, with that, there's a case that we won't handle well, which is if we end up with more than one exited thread in the list, since then all exited threads will all have the same ptid. Since inferior_thread() relies on inferior_ptid, may well return the wrong thread. My next attempt to address this, was to switch an exited thread's ptid to a globally unique "exited" ptid, which is a ptid with pid == -1 and tid == 'the thread's global GDB thread number'. Note that GDB assumes that the GDB global thread number is monotonically increasing and doesn't wrap around. (We should probably make GDB thread numbers 64-bit to prevent that happening in practice; they're currently signed 32-bit.) This attempt went a long way, but still ran into a number of issues. It was a major hack too, obviously. My next attempt is the one that I'm proposing, which is to bite the bullet and break the connection between inferior_ptid and inferior_thread(), aka the current thread. I.e., make the current thread be a global thread_info pointer that is written to directly by switch_to_thread, etc., and making inferior_thread() return that pointer, instead of having inferior_thread() lookup up the inferior_ptid thread, by ptid_t. You can look at this as a continuation of the effort of using more thread_info pointers instead of ptids when possible. By making the current thread a global thread_info pointer, we can make switch_to_thread simply write to the global thread pointer, which makes scoped_restore_current_thread able to restore back to an exited thread without relying on unrelyable ptid look ups. I.e., this makes it not a real problem to have more than one thread with the same ptid in the thread list. There will always be only one live thread with a given ptid, so code that looks up a live thread by ptid will always be able to find the right one. This change required auditing the whole codebase for places where we were writing to inferior_ptid directly to change the current thread, and change them to use switch_to_thread instead or one of its siblings, because otherwise inferior_thread() would return a thread unrelated to the changed-to inferior_ptid. That was all (hopefully) done in previous patches. After this, inferior_ptid is mainly used by target backend code. It is also relied on by a number of target methods. E.g., the target_resume interface and the memory reading routines -- we still need it there because we need to be able to access memory off of processes for which we don't have a corresponding inferior/thread object, like when handling forks. Maybe we could pass down a context explicitly to target_read_memory, etc. gdb/ChangeLog: 2020-06-18 Pedro Alves PR gdb/25412 * gdbthread.h (delete_thread, delete_thread_silent) (find_thread_ptid): Update comments. * thread.c (current_thread_): New global. (is_current_thread): Move higher, and reimplement. (inferior_thread): Reimplement. (set_thread_exited): Use bool. Add assertions. (add_thread_silent): Simplify thread-reuse handling by always calling delete_thread. (delete_thread): Remove intro comment. (find_thread_ptid): Skip exited threads. (switch_to_thread_no_regs): Write to current_thread_. (switch_to_no_thread): Check CURRENT_THREAD_ instead of INFERIOR_PTID. Clear current_thread_. --- diff --git a/gdb/ChangeLog b/gdb/ChangeLog index cd2af5c7a00..ff1fd7f4056 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,20 @@ +2020-06-18 Pedro Alves + + PR gdb/25412 + * gdbthread.h (delete_thread, delete_thread_silent) + (find_thread_ptid): Update comments. + * thread.c (current_thread_): New global. + (is_current_thread): Move higher, and reimplement. + (inferior_thread): Reimplement. + (set_thread_exited): Use bool. Add assertions. + (add_thread_silent): Simplify thread-reuse handling by always + calling delete_thread. + (delete_thread): Remove intro comment. + (find_thread_ptid): Skip exited threads. + (switch_to_thread_no_regs): Write to current_thread_. + (switch_to_no_thread): Check CURRENT_THREAD_ instead of + INFERIOR_PTID. Clear current_thread_. + 2020-06-18 Pedro Alves * aix-thread.c (pd_update): Use switch_to_thread. diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 710b4c66a65..0166b2000fb 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -415,12 +415,13 @@ extern struct thread_info *add_thread_with_info (process_stratum_target *targ, ptid_t ptid, private_thread_info *); -/* Delete an existing thread list entry. */ +/* Delete thread THREAD and notify of thread exit. If the thread is + currently not deletable, don't actually delete it but still tag it + as exited and do the notification. */ extern void delete_thread (struct thread_info *thread); -/* Delete an existing thread list entry, and be quiet about it. Used - after the process this thread having belonged to having already - exited, for example. */ +/* Like delete_thread, but be quiet about it. Used when the process + this thread belonged to has already exited, for example. */ extern void delete_thread_silent (struct thread_info *thread); /* Delete a step_resume_breakpoint from the thread database. */ @@ -460,15 +461,15 @@ extern bool in_thread_list (process_stratum_target *targ, ptid_t ptid); global id, not the system's). */ extern int valid_global_thread_id (int global_id); -/* Find thread PTID of inferior INF. */ +/* Find (non-exited) thread PTID of inferior INF. */ extern thread_info *find_thread_ptid (inferior *inf, ptid_t ptid); -/* Search function to lookup a thread by 'pid'. */ +/* Search function to lookup a (non-exited) thread by 'ptid'. */ extern struct thread_info *find_thread_ptid (process_stratum_target *targ, ptid_t ptid); -/* Search function to lookup a thread by 'ptid'. Only searches in - threads of INF. */ +/* Search function to lookup a (non-exited) thread by 'ptid'. Only + searches in threads of INF. */ extern struct thread_info *find_thread_ptid (inferior *inf, ptid_t ptid); /* Find thread by GDB global thread ID. */ diff --git a/gdb/thread.c b/gdb/thread.c index 02672f01fcf..f0722d35888 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -55,6 +55,9 @@ static int highest_thread_num; +/* The current/selected thread. */ +static thread_info *current_thread_; + /* RAII type used to increase / decrease the refcount of each thread in a given list of threads. */ @@ -78,13 +81,19 @@ private: const std::vector &m_thrds; }; +/* Returns true if THR is the current thread. */ + +static bool +is_current_thread (const thread_info *thr) +{ + return thr == current_thread_; +} struct thread_info* inferior_thread (void) { - struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid); - gdb_assert (tp); - return tp; + gdb_assert (current_thread_ != nullptr); + return current_thread_; } /* Delete the breakpoint pointed at by BP_P, if there's one. */ @@ -194,7 +203,7 @@ clear_thread_inferior_resources (struct thread_info *tp) /* Set the TP's state as exited. */ static void -set_thread_exited (thread_info *tp, int silent) +set_thread_exited (thread_info *tp, bool silent) { /* Dead threads don't need to step-over. Remove from queue. */ if (tp->step_over_next != NULL) @@ -245,7 +254,12 @@ new_thread (struct inferior *inf, ptid_t ptid) struct thread_info *last; for (last = inf->thread_list; last->next != NULL; last = last->next) - ; + gdb_assert (ptid != last->ptid + || last->state == THREAD_EXITED); + + gdb_assert (ptid != last->ptid + || last->state == THREAD_EXITED); + last->next = tp; } @@ -255,51 +269,15 @@ new_thread (struct inferior *inf, ptid_t ptid) struct thread_info * add_thread_silent (process_stratum_target *targ, ptid_t ptid) { - inferior *inf; - - thread_info *tp = find_thread_ptid (targ, ptid); - if (tp) - /* Found an old thread with the same id. It has to be dead, - otherwise we wouldn't be adding a new thread with the same id. - The OS is reusing this id --- delete it, and recreate a new - one. */ - { - /* In addition to deleting the thread, if this is the current - thread, then we need to take care that delete_thread doesn't - really delete the thread if it is inferior_ptid. Create a - new template thread in the list with an invalid ptid, switch - to it, delete the original thread, reset the new thread's - ptid, and switch to it. */ - - if (inferior_ptid == ptid) - { - thread_info *new_thr = new_thread (tp->inf, null_ptid); - - /* Make switch_to_thread not read from the thread. */ - new_thr->state = THREAD_EXITED; - switch_to_no_thread (); - - /* Now we can delete it. */ - delete_thread (tp); - - /* Now reset its ptid, and reswitch inferior_ptid to it. */ - new_thr->ptid = ptid; - new_thr->state = THREAD_STOPPED; - switch_to_thread (new_thr); - - gdb::observers::new_thread.notify (new_thr); - - /* All done. */ - return new_thr; - } + inferior *inf = find_inferior_ptid (targ, ptid); - inf = tp->inf; - - /* Just go ahead and delete it. */ - delete_thread (tp); - } - else - inf = find_inferior_ptid (targ, ptid); + /* 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 + the old thread, and create a new one. */ + thread_info *tp = find_thread_ptid (inf, ptid); + if (tp != nullptr) + delete_thread (tp); tp = new_thread (inf, ptid); gdb::observers::new_thread.notify (tp); @@ -349,14 +327,6 @@ thread_info::~thread_info () xfree (this->name); } -/* Returns true if THR is the current thread. */ - -static bool -is_current_thread (const thread_info *thr) -{ - return thr->inf == current_inferior () && thr->ptid == inferior_ptid; -} - /* See gdbthread.h. */ bool @@ -482,10 +452,7 @@ delete_thread_1 (thread_info *thr, bool silent) delete tp; } -/* Delete thread THREAD and notify of thread exit. If this is the - current thread, don't actually delete it, but tag it as exited and - do the notification. If this is the user selected thread, clear - it. */ +/* See gdbthread.h. */ void delete_thread (thread_info *thread) @@ -535,7 +502,7 @@ find_thread_ptid (process_stratum_target *targ, ptid_t ptid) struct thread_info * find_thread_ptid (inferior *inf, ptid_t ptid) { - for (thread_info *tp : inf->threads ()) + for (thread_info *tp : inf->non_exited_threads ()) if (tp->ptid == ptid) return tp; @@ -1317,7 +1284,8 @@ switch_to_thread_no_regs (struct thread_info *thread) set_current_program_space (inf->pspace); set_current_inferior (inf); - inferior_ptid = thread->ptid; + current_thread_ = thread; + inferior_ptid = current_thread_->ptid; } /* See gdbthread.h. */ @@ -1325,9 +1293,10 @@ switch_to_thread_no_regs (struct thread_info *thread) void switch_to_no_thread () { - if (inferior_ptid == null_ptid) + if (current_thread_ == nullptr) return; + current_thread_ = nullptr; inferior_ptid = null_ptid; reinit_frame_cache (); }