llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/131073.diff 2 Files Affected: - (modified) lldb/tools/debugserver/source/MacOSX/MachProcess.h (+2-2) - (modified) lldb/tools/debugserver/source/MacOSX/MachProcess.mm (+12-7) ``````````diff diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.h b/lldb/tools/debugserver/source/MacOSX/MachProcess.h index db673693a1b21..d7fa859863162 100644 --- a/lldb/tools/debugserver/source/MacOSX/MachProcess.h +++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.h @@ -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; `````````` </details> https://github.com/llvm/llvm-project/pull/131073 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
