* [PATCH 1/3] gdbserver: introduce scoped_restore_current_thread and switch_to_thread
[not found] <cover.1638380465.git.tankut.baris.aktemur@intel.com>
@ 2021-12-01 17:49 ` Tankut Baris Aktemur
2021-12-03 17:02 ` Pedro Alves
2021-12-01 17:49 ` [PATCH 2/3] gdbserver: replace direct assignments to current_thread Tankut Baris Aktemur
2021-12-01 17:49 ` [PATCH 3/3] gdbserver/linux-low: replace direct assignment " Tankut Baris Aktemur
2 siblings, 1 reply; 10+ messages in thread
From: Tankut Baris Aktemur @ 2021-12-01 17:49 UTC (permalink / raw)
To: gdb-patches
Introduce a class for restoring the current thread and a function to
switch to the given thread. This is a preparation for a refactoring
that aims to remove direct assignments to 'current_thread'.
---
gdbserver/gdbthread.h | 22 ++++++++++++++++++++++
gdbserver/inferiors.cc | 21 +++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
index 7c293b1f89d..315a4da5167 100644
--- a/gdbserver/gdbthread.h
+++ b/gdbserver/gdbthread.h
@@ -233,4 +233,26 @@ lwpid_of (const thread_info *thread)
return thread->id.lwp ();
}
+/* Switch the current thread. */
+
+void switch_to_thread (thread_info *thread);
+
+/* Save/restore current thread. */
+
+class scoped_restore_current_thread
+{
+public:
+ scoped_restore_current_thread ();
+ ~scoped_restore_current_thread ();
+
+ DISABLE_COPY_AND_ASSIGN (scoped_restore_current_thread);
+
+ /* Cancel restoring on scope exit. */
+ void dont_restore () { m_dont_restore = true; }
+
+private:
+ bool m_dont_restore = false;
+ thread_info *m_thread;
+};
+
#endif /* GDBSERVER_GDBTHREAD_H */
diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
index a636266c798..d44e40a10db 100644
--- a/gdbserver/inferiors.cc
+++ b/gdbserver/inferiors.cc
@@ -218,6 +218,14 @@ switch_to_thread (process_stratum_target *ops, ptid_t ptid)
current_thread = find_thread_ptid (ptid);
}
+/* See gdbthread.h. */
+
+void
+switch_to_thread (thread_info *thread)
+{
+ current_thread = thread;
+}
+
/* See inferiors.h. */
void
@@ -243,3 +251,16 @@ set_inferior_cwd (std::string cwd)
{
current_inferior_cwd = std::move (cwd);
}
+
+scoped_restore_current_thread::scoped_restore_current_thread ()
+{
+ m_thread = current_thread;
+}
+
+scoped_restore_current_thread::~scoped_restore_current_thread ()
+{
+ if (m_dont_restore)
+ return;
+
+ switch_to_thread (m_thread);
+}
--
2.33.1
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] gdbserver: replace direct assignments to current_thread
[not found] <cover.1638380465.git.tankut.baris.aktemur@intel.com>
2021-12-01 17:49 ` [PATCH 1/3] gdbserver: introduce scoped_restore_current_thread and switch_to_thread Tankut Baris Aktemur
@ 2021-12-01 17:49 ` Tankut Baris Aktemur
2021-12-03 17:01 ` Pedro Alves
2021-12-01 17:49 ` [PATCH 3/3] gdbserver/linux-low: replace direct assignment " Tankut Baris Aktemur
2 siblings, 1 reply; 10+ messages in thread
From: Tankut Baris Aktemur @ 2021-12-01 17:49 UTC (permalink / raw)
To: gdb-patches
Replace the direct assignments to current_thread with
switch_to_thread. Use scoped_restore_current_thread when appropriate.
There is one instance remaining in linux-low.cc's wait_for_sigstop.
This will be handled in a separate patch.
Regression-tested on X86-64 Linux using the native-gdbserver and
native-extended-gdbserver board files.
---
gdbserver/inferiors.cc | 10 ++--
gdbserver/linux-low.cc | 112 +++++++++++++------------------------
gdbserver/linux-x86-low.cc | 6 +-
gdbserver/mem-break.cc | 15 ++---
gdbserver/netbsd-low.cc | 2 +-
gdbserver/proc-service.cc | 7 +--
gdbserver/regcache.cc | 10 ++--
gdbserver/remote-utils.cc | 5 +-
gdbserver/server.cc | 13 ++---
gdbserver/target.cc | 4 +-
gdbserver/thread-db.cc | 14 ++---
gdbserver/tracepoint.cc | 8 +--
gdbserver/win32-low.cc | 6 +-
13 files changed, 76 insertions(+), 136 deletions(-)
diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
index d44e40a10db..b6cf10096af 100644
--- a/gdbserver/inferiors.cc
+++ b/gdbserver/inferiors.cc
@@ -41,7 +41,7 @@ add_thread (ptid_t thread_id, void *target_data)
all_threads.push_back (new_thread);
if (current_thread == NULL)
- current_thread = new_thread;
+ switch_to_thread (new_thread);
return new_thread;
}
@@ -99,7 +99,7 @@ remove_thread (struct thread_info *thread)
discard_queued_stop_replies (ptid_of (thread));
all_threads.remove (thread);
if (current_thread == thread)
- current_thread = NULL;
+ switch_to_thread (nullptr);
free_one_thread (thread);
}
@@ -129,7 +129,7 @@ clear_inferiors (void)
clear_dlls ();
- current_thread = NULL;
+ switch_to_thread (nullptr);
}
struct process_info *
@@ -215,7 +215,7 @@ void
switch_to_thread (process_stratum_target *ops, ptid_t ptid)
{
gdb_assert (ptid != minus_one_ptid);
- current_thread = find_thread_ptid (ptid);
+ switch_to_thread (find_thread_ptid (ptid));
}
/* See gdbthread.h. */
@@ -233,7 +233,7 @@ switch_to_process (process_info *proc)
{
int pid = pid_of (proc);
- current_thread = find_any_thread_of_pid (pid);
+ switch_to_thread (find_any_thread_of_pid (pid));
}
/* See gdbsupport/common-inferior.h. */
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d214aff7051..e79ccb5ab0e 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -430,14 +430,10 @@ linux_process_target::low_new_fork (process_info *parent, process_info *child)
void
linux_process_target::arch_setup_thread (thread_info *thread)
{
- struct thread_info *saved_thread;
-
- saved_thread = current_thread;
- current_thread = thread;
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (thread);
low_arch_setup ();
-
- current_thread = saved_thread;
}
int
@@ -672,7 +668,7 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
/* Delete the execing process and all its threads. */
mourn (proc);
- current_thread = NULL;
+ switch_to_thread (nullptr);
/* Create a new process/lwp/thread. */
proc = add_linux_process (event_pid, 0);
@@ -712,15 +708,14 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
CORE_ADDR
linux_process_target::get_pc (lwp_info *lwp)
{
- struct thread_info *saved_thread;
struct regcache *regcache;
CORE_ADDR pc;
if (!low_supports_breakpoints ())
return 0;
- saved_thread = current_thread;
- current_thread = get_lwp_thread (lwp);
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (get_lwp_thread (lwp));
regcache = get_thread_regcache (current_thread, 1);
pc = low_get_pc (regcache);
@@ -728,26 +723,22 @@ linux_process_target::get_pc (lwp_info *lwp)
if (debug_threads)
debug_printf ("pc is 0x%lx\n", (long) pc);
- current_thread = saved_thread;
return pc;
}
void
linux_process_target::get_syscall_trapinfo (lwp_info *lwp, int *sysno)
{
- struct thread_info *saved_thread;
struct regcache *regcache;
- saved_thread = current_thread;
- current_thread = get_lwp_thread (lwp);
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (get_lwp_thread (lwp));
regcache = get_thread_regcache (current_thread, 1);
low_get_syscall_trapinfo (regcache, sysno);
if (debug_threads)
debug_printf ("get_syscall_trapinfo sysno %d\n", *sysno);
-
- current_thread = saved_thread;
}
void
@@ -762,7 +753,6 @@ linux_process_target::save_stop_reason (lwp_info *lwp)
{
CORE_ADDR pc;
CORE_ADDR sw_breakpoint_pc;
- struct thread_info *saved_thread;
#if USE_SIGTRAP_SIGINFO
siginfo_t siginfo;
#endif
@@ -774,8 +764,8 @@ linux_process_target::save_stop_reason (lwp_info *lwp)
sw_breakpoint_pc = pc - low_decr_pc_after_break ();
/* breakpoint_at reads from the current thread. */
- saved_thread = current_thread;
- current_thread = get_lwp_thread (lwp);
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (get_lwp_thread (lwp));
#if USE_SIGTRAP_SIGINFO
if (ptrace (PTRACE_GETSIGINFO, lwpid_of (current_thread),
@@ -888,7 +878,6 @@ linux_process_target::save_stop_reason (lwp_info *lwp)
}
lwp->stop_pc = pc;
- current_thread = saved_thread;
return true;
}
@@ -1644,7 +1633,6 @@ linux_process_target::thread_still_has_status_pending (thread_info *thread)
&& (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT
|| lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT))
{
- struct thread_info *saved_thread;
CORE_ADDR pc;
int discard = 0;
@@ -1652,8 +1640,8 @@ linux_process_target::thread_still_has_status_pending (thread_info *thread)
pc = get_pc (lp);
- saved_thread = current_thread;
- current_thread = thread;
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (thread);
if (pc != lp->stop_pc)
{
@@ -1682,8 +1670,6 @@ linux_process_target::thread_still_has_status_pending (thread_info *thread)
}
#endif
- current_thread = saved_thread;
-
if (discard)
{
if (debug_threads)
@@ -1971,10 +1957,7 @@ linux_process_target::low_get_thread_area (int lwpid, CORE_ADDR *addrp)
bool
linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
{
- struct thread_info *saved_thread;
-
- saved_thread = current_thread;
- current_thread = get_lwp_thread (lwp);
+ switch_to_thread (get_lwp_thread (lwp));
if ((wstat == NULL
|| (WIFSTOPPED (*wstat) && WSTOPSIG (*wstat) != SIGTRAP))
@@ -2015,7 +1998,6 @@ linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
debug_printf ("Checking whether LWP %ld needs to move out of "
"the jump pad...it does\n",
lwpid_of (current_thread));
- current_thread = saved_thread;
return true;
}
@@ -2088,7 +2070,6 @@ linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
"jump pad...no\n",
lwpid_of (current_thread));
- current_thread = saved_thread;
return false;
}
@@ -2179,8 +2160,8 @@ dequeue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
bool
linux_process_target::check_stopped_by_watchpoint (lwp_info *child)
{
- struct thread_info *saved_thread = current_thread;
- current_thread = get_lwp_thread (child);
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (get_lwp_thread (child));
if (low_stopped_by_watchpoint ())
{
@@ -2188,8 +2169,6 @@ linux_process_target::check_stopped_by_watchpoint (lwp_info *child)
child->stopped_data_address = low_stopped_data_address ();
}
- current_thread = saved_thread;
-
return child->stop_reason == TARGET_STOPPED_BY_WATCHPOINT;
}
@@ -2269,7 +2248,7 @@ linux_process_target::filter_event (int lwpid, int wstat)
child_ptid = ptid_t (lwpid, lwpid);
child = add_lwp (child_ptid);
child->stopped = 1;
- current_thread = child->thread;
+ switch_to_thread (child->thread);
}
/* If we didn't find a process, one of two things presumably happened:
@@ -2548,7 +2527,7 @@ linux_process_target::wait_for_event_filtered (ptid_t wait_ptid,
*wstatp = event_child->status_pending;
event_child->status_pending_p = 0;
event_child->status_pending = 0;
- current_thread = event_thread;
+ switch_to_thread (event_thread);
return lwpid_of (event_thread);
}
@@ -2676,7 +2655,7 @@ linux_process_target::wait_for_event_filtered (ptid_t wait_ptid,
gdb_sigmask (SIG_SETMASK, &prev_mask, NULL);
- current_thread = event_thread;
+ switch_to_thread (event_thread);
return lwpid_of (event_thread);
}
@@ -2807,7 +2786,7 @@ linux_process_target::stabilize_threads ()
return;
}
- thread_info *saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
stabilizing_threads = 1;
@@ -2849,8 +2828,6 @@ linux_process_target::stabilize_threads ()
stabilizing_threads = 0;
- current_thread = saved_thread;
-
if (debug_threads)
{
thread_stuck = find_thread ([this] (thread_info *thread)
@@ -3560,7 +3537,7 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
select_event_lwp (&event_child);
/* current_thread and event_child must stay in sync. */
- current_thread = get_lwp_thread (event_child);
+ switch_to_thread (get_lwp_thread (event_child));
event_child->status_pending_p = 0;
w = event_child->status_pending;
@@ -3897,7 +3874,6 @@ linux_process_target::stuck_in_jump_pad (thread_info *thread)
void
linux_process_target::move_out_of_jump_pad (thread_info *thread)
{
- struct thread_info *saved_thread;
struct lwp_info *lwp = get_thread_lwp (thread);
int *wstat;
@@ -3910,8 +3886,8 @@ linux_process_target::move_out_of_jump_pad (thread_info *thread)
gdb_assert (lwp->stopped);
/* For gdb_breakpoint_here. */
- saved_thread = current_thread;
- current_thread = thread;
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (thread);
wstat = lwp->status_pending_p ? &lwp->status_pending : NULL;
@@ -3940,8 +3916,6 @@ linux_process_target::move_out_of_jump_pad (thread_info *thread)
}
else
lwp_suspended_inc (lwp);
-
- current_thread = saved_thread;
}
static bool
@@ -4017,9 +3991,9 @@ linux_process_target::install_software_single_step_breakpoints (lwp_info *lwp)
struct thread_info *thread = get_lwp_thread (lwp);
struct regcache *regcache = get_thread_regcache (thread, 1);
- scoped_restore save_current_thread = make_scoped_restore (¤t_thread);
+ scoped_restore_current_thread restore_thread;
- current_thread = thread;
+ switch_to_thread (thread);
std::vector<CORE_ADDR> next_pcs = low_get_next_pcs (regcache);
for (CORE_ADDR pc : next_pcs)
@@ -4067,7 +4041,6 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
int signal, siginfo_t *info)
{
struct thread_info *thread = get_lwp_thread (lwp);
- struct thread_info *saved_thread;
int ptrace_request;
struct process_info *proc = get_thread_process (thread);
@@ -4123,8 +4096,8 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
return;
}
- saved_thread = current_thread;
- current_thread = thread;
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (thread);
/* This bit needs some thinking about. If we get a signal that
we must report while a single-step reinsert is still pending,
@@ -4248,7 +4221,6 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
of coercing an 8 byte integer to a 4 byte pointer. */
(PTRACE_TYPE_ARG4) (uintptr_t) signal);
- current_thread = saved_thread;
if (errno)
perror_with_name ("resuming thread");
@@ -4445,7 +4417,6 @@ bool
linux_process_target::thread_needs_step_over (thread_info *thread)
{
struct lwp_info *lwp = get_thread_lwp (thread);
- struct thread_info *saved_thread;
CORE_ADDR pc;
struct process_info *proc = get_thread_process (thread);
@@ -4526,8 +4497,8 @@ linux_process_target::thread_needs_step_over (thread_info *thread)
return false;
}
- saved_thread = current_thread;
- current_thread = thread;
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (thread);
/* We can only step over breakpoints we know about. */
if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc))
@@ -4544,7 +4515,6 @@ linux_process_target::thread_needs_step_over (thread_info *thread)
" GDB breakpoint at 0x%s; skipping step over\n",
lwpid_of (thread), paddress (pc));
- current_thread = saved_thread;
return false;
}
else
@@ -4556,14 +4526,10 @@ linux_process_target::thread_needs_step_over (thread_info *thread)
/* We've found an lwp that needs stepping over --- return 1 so
that find_thread stops looking. */
- current_thread = saved_thread;
-
return true;
}
}
- current_thread = saved_thread;
-
if (debug_threads)
debug_printf ("Need step over [LWP %ld]? No, no breakpoint found"
" at 0x%s\n",
@@ -4576,9 +4542,7 @@ void
linux_process_target::start_step_over (lwp_info *lwp)
{
struct thread_info *thread = get_lwp_thread (lwp);
- struct thread_info *saved_thread;
CORE_ADDR pc;
- int step;
if (debug_threads)
debug_printf ("Starting step-over on LWP %ld. Stopping all threads\n",
@@ -4602,16 +4566,17 @@ linux_process_target::start_step_over (lwp_info *lwp)
shouldn't care about. */
pc = get_pc (lwp);
- saved_thread = current_thread;
- current_thread = thread;
-
- lwp->bp_reinsert = pc;
- uninsert_breakpoints_at (pc);
- uninsert_fast_tracepoint_jumps_at (pc);
+ bool step = false;
+ {
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (thread);
- step = single_step (lwp);
+ lwp->bp_reinsert = pc;
+ uninsert_breakpoints_at (pc);
+ uninsert_fast_tracepoint_jumps_at (pc);
- current_thread = saved_thread;
+ step = single_step (lwp);
+ }
resume_one_lwp (lwp, step, 0, NULL);
@@ -4624,12 +4589,12 @@ linux_process_target::finish_step_over (lwp_info *lwp)
{
if (lwp->bp_reinsert != 0)
{
- struct thread_info *saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
if (debug_threads)
debug_printf ("Finished step over.\n");
- current_thread = get_lwp_thread (lwp);
+ switch_to_thread (get_lwp_thread (lwp));
/* Reinsert any breakpoint at LWP->BP_REINSERT. Note that there
may be no breakpoint to reinsert there by now. */
@@ -4650,7 +4615,6 @@ linux_process_target::finish_step_over (lwp_info *lwp)
}
step_over_bkpt = null_ptid;
- current_thread = saved_thread;
return true;
}
else
diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
index 58ca4bab295..4955bd35d10 100644
--- a/gdbserver/linux-x86-low.cc
+++ b/gdbserver/linux-x86-low.cc
@@ -991,7 +991,7 @@ x86_linux_read_description (void)
void
x86_target::update_xmltarget ()
{
- struct thread_info *saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
/* Before changing the register cache's internal layout, flush the
contents of the current valid caches back to the threads, and
@@ -1002,12 +1002,10 @@ x86_target::update_xmltarget ()
int pid = proc->pid;
/* Look up any thread of this process. */
- current_thread = find_any_thread_of_pid (pid);
+ switch_to_thread (find_any_thread_of_pid (pid));
low_arch_setup ();
});
-
- current_thread = saved_thread;
}
/* Process qSupported query, "xmlRegisters=". Update the buffer size for
diff --git a/gdbserver/mem-break.cc b/gdbserver/mem-break.cc
index 8f2688ef24f..406594c63be 100644
--- a/gdbserver/mem-break.cc
+++ b/gdbserver/mem-break.cc
@@ -1501,13 +1501,12 @@ delete_single_step_breakpoints (struct thread_info *thread)
if (bp->type == single_step_breakpoint
&& ((struct single_step_breakpoint *) bp)->ptid == ptid_of (thread))
{
- struct thread_info *saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
- current_thread = thread;
+ switch_to_thread (thread);
*bp_link = bp->next;
release_breakpoint (proc, bp);
bp = *bp_link;
- current_thread = saved_thread;
}
else
{
@@ -1603,11 +1602,10 @@ uninsert_single_step_breakpoints (struct thread_info *thread)
reinsert breakpoint. */
if (bp->raw->refcount == 1)
{
- struct thread_info *saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
- current_thread = thread;
+ switch_to_thread (thread);
uninsert_raw_breakpoint (bp->raw);
- current_thread = saved_thread;
}
}
}
@@ -1709,11 +1707,10 @@ reinsert_single_step_breakpoints (struct thread_info *thread)
if (bp->raw->refcount == 1)
{
- struct thread_info *saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
- current_thread = thread;
+ switch_to_thread (thread);
reinsert_raw_breakpoint (bp->raw);
- current_thread = saved_thread;
}
}
}
diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
index ada92c1fd60..3c9dadd8369 100644
--- a/gdbserver/netbsd-low.cc
+++ b/gdbserver/netbsd-low.cc
@@ -310,7 +310,7 @@ netbsd_wait (ptid_t ptid, struct target_waitstatus *ourstatus,
}
if (find_thread_ptid (ptid_t (pid)))
- current_thread = find_thread_ptid (wptid);
+ switch_to_thread (find_thread_ptid (wptid));
if (code == TRAP_LWP && pst.pe_report_event == PTRACE_LWP_CREATE)
{
diff --git a/gdbserver/proc-service.cc b/gdbserver/proc-service.cc
index 4724151d283..8f3f77f69b3 100644
--- a/gdbserver/proc-service.cc
+++ b/gdbserver/proc-service.cc
@@ -105,20 +105,17 @@ ps_lgetregs (gdb_ps_prochandle_t ph, lwpid_t lwpid, prgregset_t gregset)
{
#ifdef HAVE_REGSETS
struct lwp_info *lwp;
- struct thread_info *reg_thread, *saved_thread;
struct regcache *regcache;
lwp = find_lwp_pid (ptid_t (lwpid));
if (lwp == NULL)
return PS_ERR;
- reg_thread = get_lwp_thread (lwp);
- saved_thread = current_thread;
- current_thread = reg_thread;
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (get_lwp_thread (lwp));
regcache = get_thread_regcache (current_thread, 1);
gregset_info ()->fill_function (regcache, gregset);
- current_thread = saved_thread;
return PS_OK;
#else
return PS_ERR;
diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 312f14ee9dd..304be0d5dd3 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -49,14 +49,13 @@ get_thread_regcache (struct thread_info *thread, int fetch)
if (fetch && regcache->registers_valid == 0)
{
- struct thread_info *saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
- current_thread = thread;
+ switch_to_thread (thread);
/* Invalidate all registers, to prevent stale left-overs. */
memset (regcache->register_status, REG_UNAVAILABLE,
regcache->tdesc->reg_defs.size ());
fetch_inferior_registers (regcache, -1);
- current_thread = saved_thread;
regcache->registers_valid = 1;
}
@@ -83,11 +82,10 @@ regcache_invalidate_thread (struct thread_info *thread)
if (regcache->registers_valid)
{
- struct thread_info *saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
- current_thread = thread;
+ switch_to_thread (thread);
store_inferior_registers (regcache, -1);
- current_thread = saved_thread;
}
regcache->registers_valid = 0;
diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index 8202365350a..ae1a85fa2b5 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -1099,7 +1099,6 @@ prepare_resume_reply (char *buf, ptid_t ptid, const target_waitstatus &status)
case TARGET_WAITKIND_SYSCALL_ENTRY:
case TARGET_WAITKIND_SYSCALL_RETURN:
{
- struct thread_info *saved_thread;
const char **regp;
struct regcache *regcache;
@@ -1182,7 +1181,7 @@ prepare_resume_reply (char *buf, ptid_t ptid, const target_waitstatus &status)
buf += strlen (buf);
- saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
switch_to_thread (the_target, ptid);
@@ -1273,8 +1272,6 @@ prepare_resume_reply (char *buf, ptid_t ptid, const target_waitstatus &status)
buf += strlen (buf);
current_process ()->dlls_changed = false;
}
-
- current_thread = saved_thread;
}
break;
case TARGET_WAITKIND_EXITED:
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index b1d4c92b3d9..4d686b53fd8 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1265,7 +1265,7 @@ handle_detach (char *own_buf)
cs.last_status.set_exited (0);
cs.last_ptid = ptid_t (pid);
- current_thread = NULL;
+ switch_to_thread (nullptr);
}
else
{
@@ -1687,8 +1687,7 @@ handle_qxfer_threads_proper (struct buffer *buffer)
{
client_state &cs = get_client_state ();
- scoped_restore save_current_thread
- = make_scoped_restore (¤t_thread);
+ scoped_restore_current_thread restore_thread;
scoped_restore save_current_general_thread
= make_scoped_restore (&cs.general_thread);
@@ -2223,7 +2222,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
if (strcmp ("qSymbol::", own_buf) == 0)
{
- struct thread_info *save_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
/* For qSymbol, GDB only changes the current thread if the
previous current thread was of a different process. So if
@@ -2232,15 +2231,15 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
exec in a non-leader thread. */
if (current_thread == NULL)
{
- current_thread
+ thread_info *any_thread
= find_any_thread_of_pid (cs.general_thread.pid ());
+ switch_to_thread (any_thread);
/* Just in case, if we didn't find a thread, then bail out
instead of crashing. */
if (current_thread == NULL)
{
write_enn (own_buf);
- current_thread = save_thread;
return;
}
}
@@ -2263,8 +2262,6 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
if (current_thread != NULL)
the_target->look_up_symbols ();
- current_thread = save_thread;
-
strcpy (own_buf, "OK");
return;
}
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index bfa860546a0..bac15605cb4 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -35,7 +35,7 @@ set_desired_thread ()
client_state &cs = get_client_state ();
thread_info *found = find_thread_ptid (cs.general_thread);
- current_thread = found;
+ switch_to_thread (found);
return (current_thread != NULL);
}
@@ -101,7 +101,7 @@ prepare_to_access_memory (void)
return 1;
}
- current_thread = thread;
+ switch_to_thread (thread);
cs.general_thread = ptid_of (thread);
return 0;
diff --git a/gdbserver/thread-db.cc b/gdbserver/thread-db.cc
index 9a70cdf4671..01e83571224 100644
--- a/gdbserver/thread-db.cc
+++ b/gdbserver/thread-db.cc
@@ -388,7 +388,6 @@ thread_db_get_tls_address (struct thread_info *thread, CORE_ADDR offset,
psaddr_t addr;
td_err_e err;
struct lwp_info *lwp;
- struct thread_info *saved_thread;
struct process_info *proc;
struct thread_db *thread_db;
@@ -411,8 +410,8 @@ thread_db_get_tls_address (struct thread_info *thread, CORE_ADDR offset,
if (!lwp->thread_known)
return TD_NOTHR;
- saved_thread = current_thread;
- current_thread = thread;
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (thread);
if (load_module != 0)
{
@@ -435,7 +434,6 @@ thread_db_get_tls_address (struct thread_info *thread, CORE_ADDR offset,
addr = (char *) addr + offset;
}
- current_thread = saved_thread;
if (err == TD_OK)
{
*address = (CORE_ADDR) (uintptr_t) addr;
@@ -788,7 +786,7 @@ disable_thread_event_reporting (struct process_info *proc)
if (td_ta_clear_event_p != NULL)
{
- struct thread_info *saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
td_thr_events_t events;
switch_to_process (proc);
@@ -797,8 +795,6 @@ disable_thread_event_reporting (struct process_info *proc)
in any events anymore. */
td_event_fillset (&events);
(*td_ta_clear_event_p) (thread_db->thread_agent, &events);
-
- current_thread = saved_thread;
}
}
}
@@ -894,8 +890,8 @@ thread_db_notice_clone (struct thread_info *parent_thr, ptid_t child_ptid)
/* find_one_thread calls into libthread_db which accesses memory via
the current thread. Temporarily switch to a thread we know is
stopped. */
- scoped_restore restore_current_thread
- = make_scoped_restore (¤t_thread, parent_thr);
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (parent_thr);
if (!find_one_thread (child_ptid))
warning ("Cannot find thread after clone.");
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index c01973b5e61..f176ab24393 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -3975,18 +3975,14 @@ gdb_agent_about_to_close (int pid)
if (!maybe_write_ipa_not_loaded (buf))
{
- struct thread_info *saved_thread;
-
- saved_thread = current_thread;
+ scoped_restore_current_thread restore_thread;
/* Find any thread which belongs to process PID. */
- current_thread = find_any_thread_of_pid (pid);
+ switch_to_thread (find_any_thread_of_pid (pid));
strcpy (buf, "close");
run_inferior_command (buf, strlen (buf) + 1);
-
- current_thread = saved_thread;
}
}
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index cc981d3988e..6f6fcefafbc 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -1102,7 +1102,7 @@ get_child_debug_event (DWORD *continue_status,
*ourstatus = stop->status;
current_event = stop->event;
ptid = debug_event_ptid (¤t_event);
- current_thread = find_thread_ptid (ptid);
+ switch_to_thread (find_thread_ptid (ptid));
return 1;
}
@@ -1152,7 +1152,7 @@ get_child_debug_event (DWORD *continue_status,
child_delete_thread (current_event.dwProcessId,
current_event.dwThreadId);
- current_thread = get_first_thread ();
+ switch_to_thread (get_first_thread ());
return 1;
case CREATE_PROCESS_DEBUG_EVENT:
@@ -1264,7 +1264,7 @@ get_child_debug_event (DWORD *continue_status,
ourstatus->set_spurious ();
}
else
- current_thread = find_thread_ptid (ptid);
+ switch_to_thread (find_thread_ptid (ptid));
return 1;
}
--
2.33.1
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] gdbserver/linux-low: replace direct assignment to current_thread
[not found] <cover.1638380465.git.tankut.baris.aktemur@intel.com>
2021-12-01 17:49 ` [PATCH 1/3] gdbserver: introduce scoped_restore_current_thread and switch_to_thread Tankut Baris Aktemur
2021-12-01 17:49 ` [PATCH 2/3] gdbserver: replace direct assignments to current_thread Tankut Baris Aktemur
@ 2021-12-01 17:49 ` Tankut Baris Aktemur
2021-12-03 17:01 ` Pedro Alves
2 siblings, 1 reply; 10+ messages in thread
From: Tankut Baris Aktemur @ 2021-12-01 17:49 UTC (permalink / raw)
To: gdb-patches
Use scoped_restore_current_thread and switch_to_thread in
linux_process_target::wait_for_sigstop.
---
gdbserver/linux-low.cc | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index e79ccb5ab0e..9f74de43344 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -3814,16 +3814,11 @@ lwp_is_marked_dead (struct lwp_info *lwp)
void
linux_process_target::wait_for_sigstop ()
{
- struct thread_info *saved_thread;
- ptid_t saved_tid;
+ thread_info *saved_thread = current_thread;
int wstat;
int ret;
- saved_thread = current_thread;
- if (saved_thread != NULL)
- saved_tid = saved_thread->id;
- else
- saved_tid = null_ptid; /* avoid bogus unused warning */
+ scoped_restore_current_thread restore_thread;
if (debug_threads)
debug_printf ("wait_for_sigstop: pulling events\n");
@@ -3834,8 +3829,8 @@ linux_process_target::wait_for_sigstop ()
ret = wait_for_event_filtered (minus_one_ptid, null_ptid, &wstat, __WALL);
gdb_assert (ret == -1);
- if (saved_thread == NULL || mythread_alive (saved_tid))
- current_thread = saved_thread;
+ if (saved_thread == nullptr || mythread_alive (saved_thread->id))
+ return;
else
{
if (debug_threads)
@@ -3844,7 +3839,8 @@ linux_process_target::wait_for_sigstop ()
/* We can't change the current inferior behind GDB's back,
otherwise, a subsequent command may apply to the wrong
process. */
- current_thread = NULL;
+ restore_thread.dont_restore ();
+ switch_to_thread (nullptr);
}
}
--
2.33.1
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] gdbserver: replace direct assignments to current_thread
2021-12-01 17:49 ` [PATCH 2/3] gdbserver: replace direct assignments to current_thread Tankut Baris Aktemur
@ 2021-12-03 17:01 ` Pedro Alves
2021-12-09 8:07 ` Aktemur, Tankut Baris
0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2021-12-03 17:01 UTC (permalink / raw)
To: Tankut Baris Aktemur, gdb-patches
On 2021-12-01 17:49, Tankut Baris Aktemur via Gdb-patches wrote:
> Replace the direct assignments to current_thread with
> switch_to_thread. Use scoped_restore_current_thread when appropriate.
> There is one instance remaining in linux-low.cc's wait_for_sigstop.
> This will be handled in a separate patch.
>
> Regression-tested on X86-64 Linux using the native-gdbserver and
> native-extended-gdbserver board files.
Cool, thanks!
> @@ -1971,10 +1957,7 @@ linux_process_target::low_get_thread_area (int lwpid, CORE_ADDR *addrp)
> bool
> linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
> {
> - struct thread_info *saved_thread;
> -
> - saved_thread = current_thread;
> - current_thread = get_lwp_thread (lwp);
> + switch_to_thread (get_lwp_thread (lwp));
Aren't we missing a scoped_restore here?
The rest of the patch looked fine.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] gdbserver/linux-low: replace direct assignment to current_thread
2021-12-01 17:49 ` [PATCH 3/3] gdbserver/linux-low: replace direct assignment " Tankut Baris Aktemur
@ 2021-12-03 17:01 ` Pedro Alves
2021-12-09 8:34 ` Aktemur, Tankut Baris
0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2021-12-03 17:01 UTC (permalink / raw)
To: Tankut Baris Aktemur, gdb-patches
On 2021-12-01 17:49, Tankut Baris Aktemur via Gdb-patches wrote:
> Use scoped_restore_current_thread and switch_to_thread in
> linux_process_target::wait_for_sigstop.
> ---
> gdbserver/linux-low.cc | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index e79ccb5ab0e..9f74de43344 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -3814,16 +3814,11 @@ lwp_is_marked_dead (struct lwp_info *lwp)
> void
> linux_process_target::wait_for_sigstop ()
> {
> - struct thread_info *saved_thread;
> - ptid_t saved_tid;
> + thread_info *saved_thread = current_thread;
> int wstat;
> int ret;
>
> - saved_thread = current_thread;
> - if (saved_thread != NULL)
> - saved_tid = saved_thread->id;
> - else
> - saved_tid = null_ptid; /* avoid bogus unused warning */
> + scoped_restore_current_thread restore_thread;
>
> if (debug_threads)
> debug_printf ("wait_for_sigstop: pulling events\n");
> @@ -3834,8 +3829,8 @@ linux_process_target::wait_for_sigstop ()
> ret = wait_for_event_filtered (minus_one_ptid, null_ptid, &wstat, __WALL);
> gdb_assert (ret == -1);
>
> - if (saved_thread == NULL || mythread_alive (saved_tid))
> - current_thread = saved_thread;
> + if (saved_thread == nullptr || mythread_alive (saved_thread->id))
Hmm, what if wait_for_event_filtered runs into the saved_thread exiting?
Can't that happen? I suspect it might. In that case, the saved_thread
pointer will be stale, I think. I.e., if we knew the thread pointer was
going to be valid, then what's the point of calling mythread_alive in the
first place?
> + return;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] gdbserver: introduce scoped_restore_current_thread and switch_to_thread
2021-12-01 17:49 ` [PATCH 1/3] gdbserver: introduce scoped_restore_current_thread and switch_to_thread Tankut Baris Aktemur
@ 2021-12-03 17:02 ` Pedro Alves
0 siblings, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2021-12-03 17:02 UTC (permalink / raw)
To: Tankut Baris Aktemur, gdb-patches
On 2021-12-01 17:49, Tankut Baris Aktemur via Gdb-patches wrote:
> Introduce a class for restoring the current thread and a function to
> switch to the given thread. This is a preparation for a refactoring
> that aims to remove direct assignments to 'current_thread'.
OK, thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 2/3] gdbserver: replace direct assignments to current_thread
2021-12-03 17:01 ` Pedro Alves
@ 2021-12-09 8:07 ` Aktemur, Tankut Baris
2021-12-10 15:53 ` Pedro Alves
0 siblings, 1 reply; 10+ messages in thread
From: Aktemur, Tankut Baris @ 2021-12-09 8:07 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On Friday, December 3, 2021 6:01 PM, Pedro Alves wrote:
> On 2021-12-01 17:49, Tankut Baris Aktemur via Gdb-patches wrote:
> > Replace the direct assignments to current_thread with
> > switch_to_thread. Use scoped_restore_current_thread when appropriate.
> > There is one instance remaining in linux-low.cc's wait_for_sigstop.
> > This will be handled in a separate patch.
> >
> > Regression-tested on X86-64 Linux using the native-gdbserver and
> > native-extended-gdbserver board files.
>
> Cool, thanks!
>
> > @@ -1971,10 +1957,7 @@ linux_process_target::low_get_thread_area (int lwpid, CORE_ADDR
> *addrp)
> > bool
> > linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
> > {
> > - struct thread_info *saved_thread;
> > -
> > - saved_thread = current_thread;
> > - current_thread = get_lwp_thread (lwp);
> > + switch_to_thread (get_lwp_thread (lwp));
>
> Aren't we missing a scoped_restore here?
Ah, yes, right. Here is the new diff for this hunk:
@@ -1971,10 +1957,8 @@ linux_process_target::low_get_thread_area (int lwpid, CORE_ADDR *addrp)
bool
linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
{
- struct thread_info *saved_thread;
-
- saved_thread = current_thread;
- current_thread = get_lwp_thread (lwp);
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (get_lwp_thread (lwp));
if ((wstat == NULL
|| (WIFSTOPPED (*wstat) && WSTOPSIG (*wstat) != SIGTRAP))
-Baris
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 3/3] gdbserver/linux-low: replace direct assignment to current_thread
2021-12-03 17:01 ` Pedro Alves
@ 2021-12-09 8:34 ` Aktemur, Tankut Baris
2021-12-10 15:53 ` Pedro Alves
0 siblings, 1 reply; 10+ messages in thread
From: Aktemur, Tankut Baris @ 2021-12-09 8:34 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On Friday, December 3, 2021 6:02 PM, Pedro Alves wrote:
> On 2021-12-01 17:49, Tankut Baris Aktemur via Gdb-patches wrote:
> > Use scoped_restore_current_thread and switch_to_thread in
> > linux_process_target::wait_for_sigstop.
> > ---
> > gdbserver/linux-low.cc | 16 ++++++----------
> > 1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> > index e79ccb5ab0e..9f74de43344 100644
> > --- a/gdbserver/linux-low.cc
> > +++ b/gdbserver/linux-low.cc
> > @@ -3814,16 +3814,11 @@ lwp_is_marked_dead (struct lwp_info *lwp)
> > void
> > linux_process_target::wait_for_sigstop ()
> > {
> > - struct thread_info *saved_thread;
> > - ptid_t saved_tid;
> > + thread_info *saved_thread = current_thread;
> > int wstat;
> > int ret;
> >
> > - saved_thread = current_thread;
> > - if (saved_thread != NULL)
> > - saved_tid = saved_thread->id;
> > - else
> > - saved_tid = null_ptid; /* avoid bogus unused warning */
> > + scoped_restore_current_thread restore_thread;
> >
> > if (debug_threads)
> > debug_printf ("wait_for_sigstop: pulling events\n");
> > @@ -3834,8 +3829,8 @@ linux_process_target::wait_for_sigstop ()
> > ret = wait_for_event_filtered (minus_one_ptid, null_ptid, &wstat, __WALL);
> > gdb_assert (ret == -1);
> >
> > - if (saved_thread == NULL || mythread_alive (saved_tid))
> > - current_thread = saved_thread;
> > + if (saved_thread == nullptr || mythread_alive (saved_thread->id))
>
> Hmm, what if wait_for_event_filtered runs into the saved_thread exiting?
> Can't that happen? I suspect it might. In that case, the saved_thread
> pointer will be stale, I think. I.e., if we knew the thread pointer was
> going to be valid, then what's the point of calling mythread_alive in the
> first place?
I see... Below is the updated patch.
Thanks
-Baris
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index f4b537780ca..23520c382d3 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -3826,6 +3826,8 @@ linux_process_target::wait_for_sigstop ()
else
saved_tid = null_ptid; /* avoid bogus unused warning */
+ scoped_restore_current_thread restore_thread;
+
if (debug_threads)
debug_printf ("wait_for_sigstop: pulling events\n");
@@ -3836,7 +3838,7 @@ linux_process_target::wait_for_sigstop ()
gdb_assert (ret == -1);
if (saved_thread == NULL || mythread_alive (saved_tid))
- current_thread = saved_thread;
+ return;
else
{
if (debug_threads)
@@ -3845,7 +3847,8 @@ linux_process_target::wait_for_sigstop ()
/* We can't change the current inferior behind GDB's back,
otherwise, a subsequent command may apply to the wrong
process. */
- current_thread = NULL;
+ restore_thread.dont_restore ();
+ switch_to_thread (nullptr);
}
}
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] gdbserver: replace direct assignments to current_thread
2021-12-09 8:07 ` Aktemur, Tankut Baris
@ 2021-12-10 15:53 ` Pedro Alves
0 siblings, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2021-12-10 15:53 UTC (permalink / raw)
To: Aktemur, Tankut Baris, gdb-patches
On 2021-12-09 08:07, Aktemur, Tankut Baris wrote:
> On Friday, December 3, 2021 6:01 PM, Pedro Alves wrote:
>> On 2021-12-01 17:49, Tankut Baris Aktemur via Gdb-patches wrote:
>>> Replace the direct assignments to current_thread with
>>> switch_to_thread. Use scoped_restore_current_thread when appropriate.
>>> There is one instance remaining in linux-low.cc's wait_for_sigstop.
>>> This will be handled in a separate patch.
>>>
>>> Regression-tested on X86-64 Linux using the native-gdbserver and
>>> native-extended-gdbserver board files.
>>
>> Cool, thanks!
>>
>>> @@ -1971,10 +1957,7 @@ linux_process_target::low_get_thread_area (int lwpid, CORE_ADDR
>> *addrp)
>>> bool
>>> linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
>>> {
>>> - struct thread_info *saved_thread;
>>> -
>>> - saved_thread = current_thread;
>>> - current_thread = get_lwp_thread (lwp);
>>> + switch_to_thread (get_lwp_thread (lwp));
>>
>> Aren't we missing a scoped_restore here?
>
> Ah, yes, right. Here is the new diff for this hunk:
>
This is OK. Thanks.
> @@ -1971,10 +1957,8 @@ linux_process_target::low_get_thread_area (int lwpid, CORE_ADDR *addrp)
> bool
> linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
> {
> - struct thread_info *saved_thread;
> -
> - saved_thread = current_thread;
> - current_thread = get_lwp_thread (lwp);
> + scoped_restore_current_thread restore_thread;
> + switch_to_thread (get_lwp_thread (lwp));
>
> if ((wstat == NULL
> || (WIFSTOPPED (*wstat) && WSTOPSIG (*wstat) != SIGTRAP))
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] gdbserver/linux-low: replace direct assignment to current_thread
2021-12-09 8:34 ` Aktemur, Tankut Baris
@ 2021-12-10 15:53 ` Pedro Alves
0 siblings, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2021-12-10 15:53 UTC (permalink / raw)
To: Aktemur, Tankut Baris, gdb-patches
On 2021-12-09 08:34, Aktemur, Tankut Baris wrote:
> On Friday, December 3, 2021 6:02 PM, Pedro Alves wrote:
>> On 2021-12-01 17:49, Tankut Baris Aktemur via Gdb-patches wrote:
>>> Use scoped_restore_current_thread and switch_to_thread in
>>> linux_process_target::wait_for_sigstop.
>>> ---
>>> gdbserver/linux-low.cc | 16 ++++++----------
>>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
>>> index e79ccb5ab0e..9f74de43344 100644
>>> --- a/gdbserver/linux-low.cc
>>> +++ b/gdbserver/linux-low.cc
>>> @@ -3814,16 +3814,11 @@ lwp_is_marked_dead (struct lwp_info *lwp)
>>> void
>>> linux_process_target::wait_for_sigstop ()
>>> {
>>> - struct thread_info *saved_thread;
>>> - ptid_t saved_tid;
>>> + thread_info *saved_thread = current_thread;
>>> int wstat;
>>> int ret;
>>>
>>> - saved_thread = current_thread;
>>> - if (saved_thread != NULL)
>>> - saved_tid = saved_thread->id;
>>> - else
>>> - saved_tid = null_ptid; /* avoid bogus unused warning */
>>> + scoped_restore_current_thread restore_thread;
>>>
>>> if (debug_threads)
>>> debug_printf ("wait_for_sigstop: pulling events\n");
>>> @@ -3834,8 +3829,8 @@ linux_process_target::wait_for_sigstop ()
>>> ret = wait_for_event_filtered (minus_one_ptid, null_ptid, &wstat, __WALL);
>>> gdb_assert (ret == -1);
>>>
>>> - if (saved_thread == NULL || mythread_alive (saved_tid))
>>> - current_thread = saved_thread;
>>> + if (saved_thread == nullptr || mythread_alive (saved_thread->id))
>>
>> Hmm, what if wait_for_event_filtered runs into the saved_thread exiting?
>> Can't that happen? I suspect it might. In that case, the saved_thread
>> pointer will be stale, I think. I.e., if we knew the thread pointer was
>> going to be valid, then what's the point of calling mythread_alive in the
>> first place?
>
> I see... Below is the updated patch.
>
OK.
Thanks,
Pedro Alves
> Thanks
> -Baris
>
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index f4b537780ca..23520c382d3 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -3826,6 +3826,8 @@ linux_process_target::wait_for_sigstop ()
> else
> saved_tid = null_ptid; /* avoid bogus unused warning */
>
> + scoped_restore_current_thread restore_thread;
> +
> if (debug_threads)
> debug_printf ("wait_for_sigstop: pulling events\n");
>
> @@ -3836,7 +3838,7 @@ linux_process_target::wait_for_sigstop ()
> gdb_assert (ret == -1);
>
> if (saved_thread == NULL || mythread_alive (saved_tid))
> - current_thread = saved_thread;
> + return;
> else
> {
> if (debug_threads)
> @@ -3845,7 +3847,8 @@ linux_process_target::wait_for_sigstop ()
> /* We can't change the current inferior behind GDB's back,
> otherwise, a subsequent command may apply to the wrong
> process. */
> - current_thread = NULL;
> + restore_thread.dont_restore ();
> + switch_to_thread (nullptr);
> }
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-12-10 15:53 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1638380465.git.tankut.baris.aktemur@intel.com>
2021-12-01 17:49 ` [PATCH 1/3] gdbserver: introduce scoped_restore_current_thread and switch_to_thread Tankut Baris Aktemur
2021-12-03 17:02 ` Pedro Alves
2021-12-01 17:49 ` [PATCH 2/3] gdbserver: replace direct assignments to current_thread Tankut Baris Aktemur
2021-12-03 17:01 ` Pedro Alves
2021-12-09 8:07 ` Aktemur, Tankut Baris
2021-12-10 15:53 ` Pedro Alves
2021-12-01 17:49 ` [PATCH 3/3] gdbserver/linux-low: replace direct assignment " Tankut Baris Aktemur
2021-12-03 17:01 ` Pedro Alves
2021-12-09 8:34 ` Aktemur, Tankut Baris
2021-12-10 15:53 ` Pedro Alves
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).