[Lldb-commits] [PATCH]race condition calling PushProcessIOHandler
Shawn Best
sbest at blueshiftinc.com
Wed Jul 30 16:03:47 PDT 2014
I have reworked the patch to use std::condition_variable. This particular
sync mechanism was new to me, I hope I used it correctly. Is it portable
across all target platforms/compilers? I tested on linux and OSX.
The timeout is pretty small (1ms) but seems ample based on the measurements
I made.
On Tue, Jul 29, 2014 at 9:58 PM, Matthew Gardiner <mg11 at csr.com> wrote:
> Cool, let us know how you get on!
> Matt
>
> Shawn Best wrote:
>
>> Thanks for the feedback guys.
>>
>> Studying the code, I had figured going with a straight int would in
>> practice be most efficient and not run into multi-threaded problems, even
>> if initially appearing a bit risky. I will rework it to use a
>> std::condition_variable. That will be more robust and readable.
>>
>> Shawn.
>>
>> On 7/29/2014 10:53 AM, Zachary Turner wrote:
>>
>>> Even better would be an std::condition_variable
>>>
>>>
>>> On Mon, Jul 28, 2014 at 10:30 PM, Matthew Gardiner <mg11 at csr.com
>>> <mailto:mg11 at csr.com>> wrote:
>>>
>>> Hi Shawn,
>>>
>>> I use 64-bit linux and I see this issue a lot. It usually
>>> manifests itself as the prompt just not being printed (or perhaps
>>> it just gets overwritten) - regardless - I invoke a command, and
>>> I don't see an (lldb) prompt when I should. So I'm well pleased
>>> that you are looking at this!
>>>
>>> Would it not be more robust to use a semaphore than usleep to
>>> synchronise the problematic threads?
>>>
>>> Although I've not looked too deeply into this particular issue,
>>> whenever I've seen similar races, I found that it's almost
>>> impossible to pick the right value when using a sleep command. A
>>> semaphore, though, should always ensure the waiting thread will
>>> wake precisely.
>>>
>>> I'd be happy to help to test such a fix.
>>>
>>> Matt
>>>
>>>
>>> Shawn Best wrote:
>>>
>>> Hi,
>>>
>>> I have attached a patch which addresses 3 related race
>>> conditions that cause the command line (lldb) prompt to get
>>> displayed inappropriately and make it appear it is not
>>> working correctly. This issue can be seen on linux and
>>> FreeBSD. I can also artificailly induce the problem on OSX.
>>>
>>> The issue happens when the command handler (in the main
>>> thread) issues a command such as run, step or continue.
>>> After the command finishes initiating its action, it returns
>>> up the call stack and goes back into the main command loop
>>> waiting for user input. Simultaneously, as the inferior
>>> process starts up, the MonitorChildProcess thread picks up
>>> the change and posts to the PrivateEvent thread.
>>> HandePrivateEvent() then calls PushProcessIOHandler() which
>>> will disable the command IO handler and give the inferior
>>> control of the TTY. To observe this on OSX, put a
>>>
>>> usleep(100);
>>>
>>> immediately prior the PushProcessIOHandler() in
>>> HandlePrivateEvent.
>>>
>>>
>>> My proposed solution is that after a 'run', 'step', or
>>> 'continue' command, insert a synchronization point and wait
>>> until HandlePrivateEvent knows the inferior process is
>>> running and has pushed the IO handler. One context switch
>>> (<100us) is usually all the time it takes on my machine. As
>>> an additional safety, I have a timeout (currently 1ms) so it
>>> will never hang the main thread.
>>>
>>> Any thoughts, or suggestions would be appreciated.
>>>
>>> Regards,
>>> Shawn.
>>>
>>>
>>> To report this email as spam click here
>>> <https://www.mailcontrol.com/sr/MZbqvYs5QwJvpeaetUwhCQ==>.
>>>
>>>
>>>
>>> _______________________________________________
>>> lldb-commits mailing list
>>> lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>>>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>>
>>>
>>>
>>>
>>> Member of the CSR plc group of companies. CSR plc registered in
>>> England and Wales, registered number 4187346, registered office
>>> Churchill House, Cambridge Business Park, Cowley Road, Cambridge,
>>> CB4 0WZ, United Kingdom
>>> More information can be found at www.csr.com
>>> <http://www.csr.com>. Keep up to date with CSR on our technical
>>> blog, www.csr.com/blog <http://www.csr.com/blog>, CSR people
>>> blog, www.csr.com/people <http://www.csr.com/people>, YouTube,
>>> www.youtube.com/user/CSRplc <http://www.youtube.com/user/CSRplc>,
>>> Facebook, www.facebook.com/pages/CSR/191038434253534
>>> <http://www.facebook.com/pages/CSR/191038434253534>, or follow us
>>> on Twitter at www.twitter.com/CSR_plc
>>> <http://www.twitter.com/CSR_plc>.
>>>
>>> New for 2014, you can now access the wide range of products
>>> powered by aptX at www.aptx.com <http://www.aptx.com>.
>>> _______________________________________________
>>> lldb-commits mailing list
>>> lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140730/bdaf9bd2/attachment.html>
-------------- next part --------------
diff --git a/include/lldb/Target/Process.h b/include/lldb/Target/Process.h
index 658bc75..d877270 100644
--- a/include/lldb/Target/Process.h
+++ b/include/lldb/Target/Process.h
@@ -19,6 +19,8 @@
#include <list>
#include <iosfwd>
#include <vector>
+#include <mutex>
+#include <condition_variable>
// Other libraries and framework includes
// Project includes
@@ -2654,6 +2656,34 @@ public:
bool wait_always = true,
Listener *hijack_listener = NULL);
+ //---------------------------------------------------------------------------------
+ /// Used in conjuction with WaitForProcessRunning to synchronize the main thread to
+ /// HandlePrivateEvent thread. Call this before kicking off a resume
+ //---------------------------------------------------------------------------------
+ void
+ ClearWaitForProcessRunning(void);
+
+ //--------------------------------------------------------------------------------------
+ /// Used in conjuction with WaitForProcessRunning to synchronize the main thread to
+ /// HandlePrivateEvent thread. Gets called by HandlePrivateEvent when state is running
+ //--------------------------------------------------------------------------------------
+ void
+ NotifyProcessRunning(void );
+
+ //--------------------------------------------------------------------------------------
+ /// Waits for the process state to be running within a given usec timeout.
+ ///
+ /// The main purpose of this is to implement an interlock waiting for HandlePrivateEvent
+ /// to push an IOHandler.
+ ///
+ /// @param[in] timeout_usec
+ /// The maximum time length to wait for the process to transition to the
+ /// eStateRunning state, specified in microseconds.
+ //--------------------------------------------------------------------------------------
+ void
+ WaitForProcessRunning (uint64_t timeout_usec);
+
+
lldb::StateType
WaitForStateChangedEvents (const TimeValue *timeout,
lldb::EventSP &event_sp,
@@ -3031,6 +3061,9 @@ protected:
std::string m_stderr_data;
Mutex m_profile_data_comm_mutex;
std::vector<std::string> m_profile_data;
+ bool m_process_running_sync; // used with WaitForProcessRunning() synchronization
+ std::condition_variable m_condition_process_running; // used with WaitForProcessRunning() synchronization
+ std::mutex m_mutex_process_running; // used with WaitForProcessRunning() synchronization
MemoryCache m_memory_cache;
AllocatedMemoryCache m_allocated_memory_cache;
bool m_should_detach; /// Should we detach if the process object goes away with an explicit call to Kill or Detach?
diff --git a/source/Commands/CommandObjectProcess.cpp b/source/Commands/CommandObjectProcess.cpp
index 3534df3..b506bfd 100644
--- a/source/Commands/CommandObjectProcess.cpp
+++ b/source/Commands/CommandObjectProcess.cpp
@@ -766,8 +766,15 @@ protected:
process->GetThreadList().GetThreadAtIndex(idx)->SetResumeState (eStateRunning, override_suspend);
}
}
-
+ process->ClearWaitForProcessRunning();
+
Error error(process->Resume());
+
+ // There is a race condition where this thread will return up the call stack to the main command
+ // handler and show an (lldb) prompt before HandlePrivateEvent (from PrivateStateThread) has
+ // a chance to call PushProcessIOHandler().
+ process->WaitForProcessRunning(1000);
+
if (error.Success())
{
result.AppendMessageWithFormat ("Process %" PRIu64 " resuming\n", process->GetID());
diff --git a/source/Commands/CommandObjectThread.cpp b/source/Commands/CommandObjectThread.cpp
index c58e9e8..e71d358 100644
--- a/source/Commands/CommandObjectThread.cpp
+++ b/source/Commands/CommandObjectThread.cpp
@@ -623,8 +623,14 @@ protected:
}
process->GetThreadList().SetSelectedThreadByID (thread->GetID());
+ process->ClearWaitForProcessRunning();
+
process->Resume ();
-
+
+ // There is a race condition where this thread will return up the call stack to the main command handler
+ // and show an (lldb) prompt before HandlePrivateEvent (from PrivateStateThread) has
+ // a chance to call PushProcessIOHandler().
+ process->WaitForProcessRunning(1000);
if (synchronous_execution)
{
diff --git a/source/Target/Process.cpp b/source/Target/Process.cpp
index 7e09e7e..07a77c6 100644
--- a/source/Target/Process.cpp
+++ b/source/Target/Process.cpp
@@ -691,6 +691,7 @@ Process::Process(Target &target, Listener &listener) :
m_stderr_data (),
m_profile_data_comm_mutex (Mutex::eMutexTypeRecursive),
m_profile_data (),
+ m_process_running_sync (false),
m_memory_cache (*this),
m_allocated_memory_cache (*this),
m_should_detach (false),
@@ -889,6 +890,41 @@ Process::GetNextEvent (EventSP &event_sp)
return state;
}
+void
+Process::ClearWaitForProcessRunning()
+{
+ std::lock_guard<std::mutex> lock(m_mutex_process_running);
+ m_process_running_sync = false;
+}
+
+void
+Process::NotifyProcessRunning(void )
+{
+ {
+ std::lock_guard<std::mutex> lock(m_mutex_process_running);
+ m_process_running_sync = true;
+ }
+ m_condition_process_running.notify_all();
+}
+
+void
+Process::WaitForProcessRunning (uint64_t timeout_usec)
+{
+ std::unique_lock<std::mutex> lock(m_mutex_process_running);
+
+ m_condition_process_running.wait_for(lock,
+ std::chrono::microseconds(timeout_usec),
+ [this](){ return m_process_running_sync;});
+
+ Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
+ if(log)
+ {
+ if(m_process_running_sync)
+ log->Printf ("Process::%s pid %" PRIu64 ": SUCCESS", __FUNCTION__, GetID ());
+ else
+ log->Printf ("Process::%s pid %" PRIu64 " (timeout=%" PRIu64 "): FAIL", __FUNCTION__, GetID (), timeout_usec);
+ }
+}
StateType
Process::WaitForProcessToStop (const TimeValue *timeout, lldb::EventSP *event_sp_ptr, bool wait_always, Listener *hijack_listener)
@@ -3878,6 +3914,7 @@ Process::HandlePrivateEvent (EventSP &event_sp)
// as this means the curses GUI is in use...
if (!GetTarget().GetDebugger().IsForwardingEvents())
PushProcessIOHandler ();
+ NotifyProcessRunning();
}
else if (StateIsStoppedState(new_state, false))
{
diff --git a/source/Target/Target.cpp b/source/Target/Target.cpp
index f959010..c91ebc3 100644
--- a/source/Target/Target.cpp
+++ b/source/Target/Target.cpp
@@ -2419,8 +2419,15 @@ Target::Launch (Listener &listener, ProcessLaunchInfo &launch_info)
if (!synchronous_execution)
m_process_sp->RestoreProcessEvents ();
+ m_process_sp->ClearWaitForProcessRunning();
+
error = m_process_sp->PrivateResume();
-
+
+ // there is a race condition where this thread will return up the call stack to the main command
+ // handler and show an (lldb) prompt before HandlePrivateEvent (from PrivateStateThread) has
+ // a chance to call PushProcessIOHandler()
+ m_process_sp->WaitForProcessRunning(1000);
+
if (error.Success())
{
if (synchronous_execution)
More information about the lldb-commits
mailing list