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
> <revi...@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
>
>
>
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
