Clean up MachTask.mm's handling of m_exception_thread.

[jimingham]

This was getting joined in ShutDownExcecptionThread (sic) but not cleared. So this function was not safe to call twice, since you aren't supposed to join a thread twice. Sadly, this was called in MachTask::Clear and MachProcess::Destroy, which are both called when you tell debugserver to detach.

This didn't seem to cause problems IRL, but the most recent ASAN detects this as an error and calls ASAN::Die, which was causing all the tests that ran detach to fail.

I fixed that by moving the clear & test for m_exception_thread to ShutDownExceptionThread. I also fixed the spelling of that routine. And that routine was claiming to return a kern_return_t which no one was checking. It actually returns a kern_return_t if there was a Mach failure and a Posix error if there was a join failure. Since there's really nothing you can do but exit if this fails, which is always what you are in the process of doing when you call this, and since we have already done all the useful logging in ShutDownExceptionThread, I just removed the return value.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

This was getting joined in ShutDownExcecptionThread (sic) but not cleared. So this function was not safe to call twice, since you aren't supposed to join a thread twice. Sadly, this was called in MachTask::Clear and MachProcess::Destroy, which are both called when you tell debugserver to detach.

This didn't seem to cause problems IRL, but the most recent ASAN detects this as an error and calls ASAN::Die, which was causing all the tests that ran detach to fail.

I fixed that by moving the clear & test for m_exception_thread to ShutDownExceptionThread. I also fixed the spelling of that routine. And that routine was claiming to return a kern_return_t which no one was checking. It actually returns a kern_return_t if there was a Mach failure and a Posix error if there was a join failure. Since there's really nothing you can do but exit if this fails, which is always what you are in the process of doing when you call this, and since we have already done all the useful logging in ShutDownExceptionThread, I just removed the return value.

---

Full diff: https://github.com/llvm/llvm-project/pull/167994.diff

3 Files Affected:

• (modified) lldb/tools/debugserver/source/MacOSX/MachProcess.mm (+1-1)
• (modified) lldb/tools/debugserver/source/MacOSX/MachTask.h (+1-1)
• (modified) lldb/tools/debugserver/source/MacOSX/MachTask.mm (+8-5)

diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
index 8df3f29a7e825..3b875e61a268d 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
@@ -1739,7 +1739,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
     ReplyToAllExceptions();
   }
 
-  m_task.ShutDownExcecptionThread();
+  m_task.ShutDownExceptionThread();
 
   // Detach from our process
   errno = 0;
diff --git a/lldb/tools/debugserver/source/MacOSX/MachTask.h b/lldb/tools/debugserver/source/MacOSX/MachTask.h
index 915f65a8160ee..40fdbe9eeeb24 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachTask.h
+++ b/lldb/tools/debugserver/source/MacOSX/MachTask.h
@@ -68,7 +68,7 @@ class MachTask {
   bool ExceptionPortIsValid() const;
   kern_return_t SaveExceptionPortInfo();
   kern_return_t RestoreExceptionPortInfo();
-  kern_return_t ShutDownExcecptionThread();
+  void ShutDownExceptionThread();
 
   bool StartExceptionThread(
       const RNBContext::IgnoredExceptions &ignored_exceptions, DNBError &err);
diff --git a/lldb/tools/debugserver/source/MacOSX/MachTask.mm b/lldb/tools/debugserver/source/MacOSX/MachTask.mm
index e5bbab830b187..22ad0c407de42 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachTask.mm
+++ b/lldb/tools/debugserver/source/MacOSX/MachTask.mm
@@ -145,10 +145,8 @@
 //----------------------------------------------------------------------
 void MachTask::Clear() {
   // Do any cleanup needed for this task
-  if (m_exception_thread)
-    ShutDownExcecptionThread();
+  ShutDownExceptionThread();
   m_task = TASK_NULL;
-  m_exception_thread = 0;
   m_exception_port = MACH_PORT_NULL;
   m_exec_will_be_suspended = false;
   m_do_double_resume = false;
@@ -685,8 +683,11 @@ static void get_threads_profile_data(DNBProfileDataScanType scanType,
   return false;
 }
 
-kern_return_t MachTask::ShutDownExcecptionThread() {
+void MachTask::ShutDownExceptionThread() {
   DNBError err;
+  
+  if (!m_exception_thread)
+    return;
 
   err = RestoreExceptionPortInfo();
 
@@ -702,6 +703,8 @@ static void get_threads_profile_data(DNBProfileDataScanType scanType,
   if (DNBLogCheckLogBit(LOG_TASK) || err.Fail())
     err.LogThreaded("::pthread_join ( thread = %p, value_ptr = NULL)",
                     m_exception_thread);
+                    
+  m_exception_thread = nullptr;
 
   // Deallocate our exception port that we used to track our child process
   mach_port_t task_self = mach_task_self();
@@ -713,7 +716,7 @@ static void get_threads_profile_data(DNBProfileDataScanType scanType,
   m_exec_will_be_suspended = false;
   m_do_double_resume = false;
 
-  return err.Status();
+  return;
 }
 
 void *MachTask::ExceptionThread(void *arg) {

[adrian-prantl]

🤔

[jimingham]

No idea, it's been that way since forever.

[jasonmolenda]

LGTM.