[Lldb-commits] [lldb] r235579 - [NativeProcessLinux] Fix race condition during inferior thread creation
Pavel Labath
labath at google.com
Thu Apr 23 02:04:36 PDT 2015
Author: labath
Date: Thu Apr 23 04:04:35 2015
New Revision: 235579
URL: http://llvm.org/viewvc/llvm-project?rev=235579&view=rev
Log:
[NativeProcessLinux] Fix race condition during inferior thread creation
The following situation occured if we were stopping a process (due to breakpoint, watchpoint, ...
hit) while a new thread was being created.
- process has two threads: A and B.
- thread A hits a breakpoint: we send a STOP signal to thread B and register a callback with
ThreadStateCoordinator to send a stop notification after the thread stops.
- thread B stops, but not due to the SIGSTOP, but on a thread creation event (of a new thread C).
We are unaware of our desire to stop, so we queue ThreadStopped and RequestResume operations
with TSC, so the thread can continue running.
- TSC receives the ThreadStopped event, sees that all threads are stopped and fires the delayed
stop notification.
- immediately after that TSC gets the RequestResume operation, so it resumes the thread.
At this point the state is inconsistent because LLDB thinks the process is stopped and will start
issuing commands to it, but one of the threads is in fact running. Things eventually break.
I address this problem by omitting the two TSC events altogether and Resuming the thread B
directly. This way the short stop is invisible to the TSC and the delayed notification will not
fire. We will fire the notification when we actually process the SIGSTOP on thread B.
When we get the initial SIGSTOP for thread C, we also resume the thread and send a
ThreadWasCreated message (is_stopped = false) to the TSC. This way, the TSC can stop the thread
on its own and handle the stop event later. This way the state of the new thread is correctly
handled as well (thanks Chaoren for the idea).
This patch also removes the synchronisation between the thread creation notifications on threads
B and C. The need for this synchronisation is unclear (the comments seem to hint that the new
thread is "fully created" only after we process both events, but I have noticed no regressions in
treating it as "created" even after just processing the initial C event), but it is a source for
many kinds of obscure races, since it introduces a new thread state "Launching" and the rest of
the code does not handle this state at all (what happens if we get a resume request from LLDB
while this thread is launching? what happens if we get a stop request? etc.).
This fixes the "spurious $O packet" problem in TestPrintStackTraces.py. However, the test remains
disabled on i386 due to the VDSO issue.
Test Plan:
TestPrintStackTraces works on x86_64. No regressions in the rest of the test suite.
Reviewers: vharron, chaoren
Subscribers: lldb-commits
Differential Revision: http://reviews.llvm.org/D9145
Modified:
lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h
lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.cpp
lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.h
Modified: lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp?rev=235579&r1=235578&r2=235579&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp (original)
+++ lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp Thu Apr 23 04:04:35 2015
@@ -2260,62 +2260,25 @@ NativeProcessLinux::MonitorSIGTRAP(const
case (SIGTRAP | (PTRACE_EVENT_CLONE << 8)):
{
- lldb::tid_t tid = LLDB_INVALID_THREAD_ID;
+ // This is the notification on the parent thread which informs us of new thread
+ // creation. We are not interested in these events at this point (an interesting use
+ // case would be to stop the process upon thread creation), so we just resume the thread.
+ // We will pickup the new thread when we get its SIGSTOP notification.
- // The main thread is stopped here.
- if (thread_sp)
- std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetStoppedBySignal (SIGTRAP);
- NotifyThreadStop (pid);
-
- unsigned long event_message = 0;
- if (GetEventMessage (pid, &event_message).Success())
+ if (log)
{
- tid = static_cast<lldb::tid_t> (event_message);
- if (log)
+ unsigned long event_message = 0;
+ if (GetEventMessage (pid, &event_message).Success())
+ {
+ lldb::tid_t tid = static_cast<lldb::tid_t> (event_message);
log->Printf ("NativeProcessLinux::%s() pid %" PRIu64 " received thread creation event for tid %" PRIu64, __FUNCTION__, pid, tid);
- // If we don't track the thread yet: create it, mark as stopped.
- // If we do track it, this is the wait we needed. Now resume the new thread.
- // In all cases, resume the current (i.e. main process) thread.
- bool created_now = false;
- NativeThreadProtocolSP new_thread_sp = GetOrCreateThread (tid, created_now);
- assert (new_thread_sp.get() && "failed to get or create the tracking data for newly created inferior thread");
-
- // If the thread was already tracked, it means the created thread already received its SI_USER notification of creation.
- if (!created_now)
- {
- // We can now resume the newly created thread since it is fully created.
- NotifyThreadCreateStopped (tid);
- m_coordinator_up->RequestThreadResume (tid,
- [=](lldb::tid_t tid_to_resume, bool supress_signal)
- {
- std::static_pointer_cast<NativeThreadLinux> (new_thread_sp)->SetRunning ();
- return Resume (tid_to_resume, LLDB_INVALID_SIGNAL_NUMBER);
- },
- CoordinatorErrorHandler);
}
else
- {
- // Mark the thread as currently launching. Need to wait for SIGTRAP clone on the main thread before
- // this thread is ready to go.
- std::static_pointer_cast<NativeThreadLinux> (new_thread_sp)->SetLaunching ();
- }
- }
- else
- {
- if (log)
log->Printf ("NativeProcessLinux::%s() pid %" PRIu64 " received thread creation event but GetEventMessage failed so we don't know the new tid", __FUNCTION__, pid);
}
- // In all cases, we can resume the main thread here.
- m_coordinator_up->RequestThreadResume (pid,
- [=](lldb::tid_t tid_to_resume, bool supress_signal)
- {
- std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetRunning ();
- return Resume (tid_to_resume, LLDB_INVALID_SIGNAL_NUMBER);
- },
- CoordinatorErrorHandler);
-
+ Resume (pid, LLDB_INVALID_SIGNAL_NUMBER);
break;
}
@@ -2640,31 +2603,12 @@ NativeProcessLinux::MonitorSignal(const
log->Printf ("NativeProcessLinux::%s() pid = %" PRIu64 " tid %" PRIu64 ": new thread notification",
__FUNCTION__, GetID (), pid);
- // Did we already create the thread?
- bool created_now = false;
- thread_sp = GetOrCreateThread (pid, created_now);
- assert (thread_sp.get() && "failed to get or create the tracking data for newly created inferior thread");
-
- // If the thread was already tracked, it means the main thread already received its SIGTRAP for the create.
- if (!created_now)
- {
- // We can now resume the newly created thread since it is fully created.
- NotifyThreadCreateStopped (pid);
- m_coordinator_up->RequestThreadResume (pid,
- [=](lldb::tid_t tid_to_resume, bool supress_signal)
- {
- std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetRunning ();
- return Resume (tid_to_resume, LLDB_INVALID_SIGNAL_NUMBER);
- },
- CoordinatorErrorHandler);
- }
- else
- {
- // Mark the thread as currently launching. Need to wait for SIGTRAP clone on the main thread before
- // this thread is ready to go.
- std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetLaunching ();
- }
-
+ thread_sp = AddThread(pid);
+ assert (thread_sp.get() && "failed to create the tracking data for newly created inferior thread");
+ // We can now resume the newly created thread.
+ std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetRunning ();
+ Resume (pid, LLDB_INVALID_SIGNAL_NUMBER);
+ m_coordinator_up->NotifyThreadCreate (pid, false, CoordinatorErrorHandler);
// Done handling.
return;
}
@@ -4109,48 +4053,6 @@ NativeProcessLinux::AddThread (lldb::tid
return thread_sp;
}
-NativeThreadProtocolSP
-NativeProcessLinux::GetOrCreateThread (lldb::tid_t thread_id, bool &created)
-{
- Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));
-
- Mutex::Locker locker (m_threads_mutex);
- if (log)
- {
- log->Printf ("NativeProcessLinux::%s pid %" PRIu64 " get/create thread with tid %" PRIu64,
- __FUNCTION__,
- GetID (),
- thread_id);
- }
-
- // Retrieve the thread if it is already getting tracked.
- NativeThreadProtocolSP thread_sp = MaybeGetThreadNoLock (thread_id);
- if (thread_sp)
- {
- if (log)
- log->Printf ("NativeProcessLinux::%s pid %" PRIu64 " tid %" PRIu64 ": thread already tracked, returning",
- __FUNCTION__,
- GetID (),
- thread_id);
- created = false;
- return thread_sp;
-
- }
-
- // Create the thread metadata since it isn't being tracked.
- if (log)
- log->Printf ("NativeProcessLinux::%s pid %" PRIu64 " tid %" PRIu64 ": thread didn't exist, tracking now",
- __FUNCTION__,
- GetID (),
- thread_id);
-
- thread_sp.reset (new NativeThreadLinux (this, thread_id));
- m_threads.push_back (thread_sp);
- created = true;
-
- return thread_sp;
-}
-
Error
NativeProcessLinux::FixupBreakpointPCAsNeeded (NativeThreadProtocolSP &thread_sp)
{
Modified: lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h?rev=235579&r1=235578&r2=235579&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h (original)
+++ lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h Thu Apr 23 04:04:35 2015
@@ -322,9 +322,6 @@ namespace process_linux {
NativeThreadProtocolSP
AddThread (lldb::tid_t thread_id);
- NativeThreadProtocolSP
- GetOrCreateThread (lldb::tid_t thread_id, bool &created);
-
Error
GetSoftwareBreakpointPCOffset (NativeRegisterContextSP context_sp, uint32_t &actual_opcode_size);
Modified: lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.cpp?rev=235579&r1=235578&r2=235579&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.cpp (original)
+++ lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.cpp Thu Apr 23 04:04:35 2015
@@ -300,20 +300,6 @@ NativeThreadLinux::RemoveWatchpoint (lld
}
void
-NativeThreadLinux::SetLaunching ()
-{
- const StateType new_state = StateType::eStateLaunching;
- MaybeLogStateChange (new_state);
- m_state = new_state;
-
- // Also mark it as stopped since launching temporarily stops the newly created thread
- // in the ptrace machinery.
- m_stop_info.reason = StopReason::eStopReasonSignal;
- m_stop_info.details.signal.signo = SIGSTOP;
-}
-
-
-void
NativeThreadLinux::SetRunning ()
{
const StateType new_state = StateType::eStateRunning;
Modified: lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.h?rev=235579&r1=235578&r2=235579&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.h (original)
+++ lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.h Thu Apr 23 04:04:35 2015
@@ -54,9 +54,6 @@ namespace process_linux {
// Interface for friend classes
// ---------------------------------------------------------------------
void
- SetLaunching ();
-
- void
SetRunning ();
void
More information about the lldb-commits
mailing list