https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/131073
>From 48ab516f56328fe101fe3fe1cd22a06ec1a033e2 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jo...@devlieghere.com>
Date: Wed, 12 Mar 2025 22:07:41 -0700
Subject: [PATCH] [lldb] Synchronize interrupt and resume signals in
debugserver
This PR fixes a race condition in debugserver where the main thread
calls MachProcess::Interrupt, setting `m_sent_interrupt_signo` while
the exception monitoring thread is checking value of the variable.
I was on the fence between introducing a new mutex and reusing the
existing exception mutex. With the notable exception of
MachProcess::Interrupt, all the other places where we were already
locking this mutex before accessing the variable. I renamed the mutex to
make it clear that it's now protecting more than the exception messages.
Jason, while investigating a real issue, had a suspicion there was race
condition related to interrupts and I was able to narrow it down by
building debugserver with TSan.
---
.../debugserver/source/MacOSX/MachProcess.h | 6 +++---
.../debugserver/source/MacOSX/MachProcess.mm | 19 ++++++++++++-------
2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.h
b/lldb/tools/debugserver/source/MacOSX/MachProcess.h
index db673693a1b21..ec0a13b482267 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.h
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.h
@@ -427,7 +427,7 @@ class MachProcess {
m_profile_data_mutex; // Multithreaded protection for profile info data
std::vector<std::string>
m_profile_data; // Profile data, must be protected by
m_profile_data_mutex
- PThreadEvent m_profile_events; // Used for the profile thread cancellable
wait
+ PThreadEvent m_profile_events; // Used for the profile thread cancellable
wait
DNBThreadResumeActions m_thread_actions; // The thread actions for the
current
// MachProcess::Resume() call
MachException::Message::collection m_exception_messages; // A collection of
@@ -435,8 +435,8 @@ class MachProcess {
// caught when
// listening to the
// exception port
- PThreadMutex m_exception_messages_mutex; // Multithreaded protection for
- // m_exception_messages
+ PThreadMutex m_exception_and_signal_mutex; // Multithreaded protection for
+ // exceptions and signals.
MachThreadList m_thread_list; // A list of threads that is maintained/updated
// after each stop
diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
index a2179bf2f91e5..7dcc04c07bbff 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
@@ -528,7 +528,7 @@ static bool FBSAddEventDataToOptions(NSMutableDictionary
*options,
m_profile_data_mutex(PTHREAD_MUTEX_RECURSIVE), m_profile_data(),
m_profile_events(0, eMachProcessProfileCancel), m_thread_actions(),
m_exception_messages(),
- m_exception_messages_mutex(PTHREAD_MUTEX_RECURSIVE), m_thread_list(),
+ m_exception_and_signal_mutex(PTHREAD_MUTEX_RECURSIVE), m_thread_list(),
m_activities(), m_state(eStateUnloaded),
m_state_mutex(PTHREAD_MUTEX_RECURSIVE), m_events(0, kAllEventsMask),
m_private_events(0, kAllEventsMask), m_breakpoints(), m_watchpoints(),
@@ -1338,8 +1338,11 @@ static bool mach_header_validity_test(uint32_t magic,
uint32_t cputype) {
m_stop_count = 0;
m_thread_list.Clear();
{
- PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+ PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
m_exception_messages.clear();
+ m_sent_interrupt_signo = 0;
+ m_auto_resume_signo = 0;
+
}
m_activities.Clear();
StopProfileThread();
@@ -1573,6 +1576,8 @@ static uint64_t bits(uint64_t value, uint32_t msbit,
uint32_t lsbit) {
}
bool MachProcess::Interrupt() {
+ PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
+
nub_state_t state = GetState();
if (IsRunning(state)) {
if (m_sent_interrupt_signo == 0) {
@@ -1728,7 +1733,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit,
uint32_t lsbit) {
m_thread_actions.Append(thread_action);
m_thread_actions.SetDefaultThreadActionIfNeeded(eStateRunning, 0);
- PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+ PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
ReplyToAllExceptions();
}
@@ -1854,7 +1859,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit,
uint32_t lsbit) {
}
void MachProcess::ReplyToAllExceptions() {
- PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+ PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
if (!m_exception_messages.empty()) {
MachException::Message::iterator pos;
MachException::Message::iterator begin = m_exception_messages.begin();
@@ -1888,7 +1893,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit,
uint32_t lsbit) {
}
}
void MachProcess::PrivateResume() {
- PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+ PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
m_auto_resume_signo = m_sent_interrupt_signo;
if (m_auto_resume_signo)
@@ -2290,7 +2295,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit,
uint32_t lsbit) {
// data has already been copied.
void MachProcess::ExceptionMessageReceived(
const MachException::Message &exceptionMessage) {
- PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+ PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
if (m_exception_messages.empty())
m_task.Suspend();
@@ -2304,7 +2309,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit,
uint32_t lsbit) {
task_t MachProcess::ExceptionMessageBundleComplete() {
// We have a complete bundle of exceptions for our child process.
- PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
+ PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
DNBLogThreadedIf(LOG_EXCEPTIONS, "%s: %llu exception messages.",
__PRETTY_FUNCTION__, (uint64_t)m_exception_messages.size());
bool auto_resume = false;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
