[Lldb-commits] [PATCH] D75004: Fix a race between lldb's packet timeout and killing the profile thread

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Lots of change for something that might be fixed much easier:

Alt way: Why not just set m_profile_enabled to false in StopProfileThread() and let loop exit on next iteration? Code changes would be much smaller. All comments above are marked with "alt way:" for this solution



================
Comment at: lldb/test/API/macosx/profile_vrs_detach/TestDetachVrsProfile.py:27
+    def test_profile_and_detach(self):
+        """There can be many tests in a test case - describe this test here."""
+        self.build()
----------------
fix comment?


================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.h:341-346
+
+  enum {
+    eMachProcessProfileNone = 0,
+    eMachProcessProfileCancel = (1 << 0)
+  };
+
----------------
Alt way (see main comment): remove these lines


================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.h:385
       m_profile_data; // Profile data, must be protected by m_profile_data_mutex
-
+  PThreadEvent m_profile_events; // Used for the profile thread cancellable wait  
   DNBThreadResumeActions m_thread_actions; // The thread actions for the current
----------------
Alt way: remove


================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:28
 #include <sys/sysctl.h>
+#include <sys/time.h>
 #include <sys/types.h>
----------------
remove


================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:34
 #include <algorithm>
+#include <chrono>
 #include <map>
----------------
remove


================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:490
       m_profile_data_mutex(PTHREAD_MUTEX_RECURSIVE), m_profile_data(),
+      m_profile_events(0, eMachProcessProfileCancel),
       m_thread_actions(), m_exception_messages(),
----------------
alt way: remove


================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:1326
+    return;
+  m_profile_events.SetEvents(eMachProcessProfileCancel);
+  pthread_join(m_profile_thread, NULL);
----------------
alt way: 

```
m_profile_enabled = false;
```



================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:1329
+  m_profile_thread = NULL;
+  m_profile_events.ResetEvents(eMachProcessProfileCancel);
+}
----------------
alt way: remove


================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:2511-2538
   while (proc->IsProfilingEnabled()) {
     nub_state_t state = proc->GetState();
     if (state == eStateRunning) {
       std::string data =
           proc->Task().GetProfileData(proc->GetProfileScanType());
       if (!data.empty()) {
         proc->SignalAsyncProfileData(data.c_str());
----------------
Alt way: revert all changes here, when we call StopProfileThread, it will set m_profile_enabled = false and this loop will naturally exit.

Your way: Move the conversion of profile interval out of the loop?

```
 timespec ts;
 using namespace std::chrono;
 std::chrono::microseconds dur(proc->ProfileInterval());
 const auto dur_secs = duration_cast<seconds>(dur);
 const auto dur_usecs = dur % std::chrono::seconds(1);
  while (proc->IsProfilingEnabled()) {
    nub_state_t state = proc->GetState();
    if (state == eStateRunning) {
      std::string data =
          proc->Task().GetProfileData(proc->GetProfileScanType());
      if (!data.empty()) {
        proc->SignalAsyncProfileData(data.c_str());
      }
    } else if ((state == eStateUnloaded) || (state == eStateDetached) ||
               (state == eStateUnloaded)) {
      // Done. Get out of this thread.
      break;
    }
    DNBTimer::OffsetTimeOfDay(&ts, dur_secs.count(), dur_usecs.count());
    // Exit if requested.
    if (proc->m_profile_events.WaitForSetEvents(eMachProcessProfileCancel, &ts))
      break;
  }
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75004/new/

https://reviews.llvm.org/D75004