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

[Jim Ingham via lldb-commits]

I don’t understand your suggestion.  The point of this change was to get the profile loop to exit without waiting the full sleep time.  That time is generally pretty long (1 second or thereabouts) and so if you have the main debugserver thread wait on the profile thread’s timeout before replying to the detach packet, then lldb might have already timed out waiting for the packet reply by the time it does so.

You could also fix this by not doing the pthread_join but instead have the detaching finish independently and then let the profile thread exit after the fact, but that seems like asking for trouble.  Or you could reply to the detach right away, and then wait on the profile thread to exit as debugserver is going down.  But the current code is not at all set up to do that.  

Anyway, this solution has the benefit of being exactly what you want to have happen - the profile thread stops its wait sleep immediately when told to exit.  And it relies on well tested mechanisms, and doesn’t seem terribly intrusive.

BTW, I thought about doing the time calculation outside the profile thread loop, but I couldn’t see anything that said you have to stop and restart the profile loop when you change the timeout.  So it didn’t seem safe to only calculate the timeout once.  However, if we want to require that you stop and start the profile thread when you change its wait interval it would be fine to do that.

Jim


> On Feb 21, 2020, at 6:27 PM, Greg Clayton via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> 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
> 
> 
>