diff --git a/include/lldb/Interpreter/ScriptInterpreterPython.h b/include/lldb/Interpreter/ScriptInterpreterPython.h index b254046..c400d26 100644 --- a/include/lldb/Interpreter/ScriptInterpreterPython.h +++ b/include/lldb/Interpreter/ScriptInterpreterPython.h @@ -228,10 +228,7 @@ private: FILE* wait_msg_handle = NULL); ~Locker (); - - static bool - CurrentThreadHasPythonLock (); - + private: bool @@ -245,17 +242,14 @@ private: bool DoTearDownSession (); - - static bool - TryGetPythonLock (uint32_t seconds_to_wait); - + static void ReleasePythonLock (); - + bool m_need_session; - bool m_release_lock; ScriptInterpreterPython *m_python_interpreter; FILE* m_tmp_fh; + PyGILState_STATE m_GILState; }; static size_t @@ -278,7 +272,6 @@ private: bool m_session_is_active; bool m_pty_slave_is_open; bool m_valid_session; - }; } // namespace lldb_private diff --git a/scripts/Python/build-swig-Python.sh b/scripts/Python/build-swig-Python.sh index 76c5383..de4fa61 100755 --- a/scripts/Python/build-swig-Python.sh +++ b/scripts/Python/build-swig-Python.sh @@ -281,7 +281,7 @@ fi # Build the SWIG C++ wrapper file for Python. -$SWIG -c++ -shadow -python -I"/usr/include" -I"${SRC_ROOT}/include" -I./. -outdir "${CONFIG_BUILD_DIR}" -o "${swig_output_file}" "${swig_input_file}" +$SWIG -c++ -shadow -python -threads -I"/usr/include" -I"${SRC_ROOT}/include" -I./. -outdir "${CONFIG_BUILD_DIR}" -o "${swig_output_file}" "${swig_input_file}" # Implement the iterator protocol and/or eq/ne operators for some lldb objects. # Append global variable to lldb Python module. diff --git a/source/Interpreter/ScriptInterpreterPython.cpp b/source/Interpreter/ScriptInterpreterPython.cpp index 39c1117..2398152 100644 --- a/source/Interpreter/ScriptInterpreterPython.cpp +++ b/source/Interpreter/ScriptInterpreterPython.cpp @@ -41,7 +41,6 @@ using namespace lldb; using namespace lldb_private; - static ScriptInterpreter::SWIGInitCallback g_swig_init_callback = NULL; static ScriptInterpreter::SWIGBreakpointCallbackFunction g_swig_breakpoint_callback = NULL; static ScriptInterpreter::SWIGPythonTypeScriptCallbackFunction g_swig_typescript_callback = NULL; @@ -117,73 +116,18 @@ _check_and_flush (FILE *stream) return fflush (stream) || prev_fail ? EOF : 0; } -static Predicate & -PythonMutexPredicate () -{ - static lldb_private::Predicate g_interpreter_is_running (LLDB_INVALID_THREAD_ID); - return g_interpreter_is_running; -} - -bool -ScriptInterpreterPython::Locker::CurrentThreadHasPythonLock () -{ - TimeValue timeout; - - timeout = TimeValue::Now(); // Don't wait any time. - - return PythonMutexPredicate().WaitForValueEqualTo (Host::GetCurrentThreadID(), &timeout, NULL); -} - -bool -ScriptInterpreterPython::Locker::TryGetPythonLock (uint32_t seconds_to_wait) -{ - - TimeValue timeout; - - if (seconds_to_wait != UINT32_MAX) - { - timeout = TimeValue::Now(); - timeout.OffsetWithSeconds (seconds_to_wait); - } - - return PythonMutexPredicate().WaitForValueEqualToAndSetValueTo (LLDB_INVALID_THREAD_ID, - Host::GetCurrentThreadID(), &timeout, NULL); -} - -void -ScriptInterpreterPython::Locker::ReleasePythonLock () -{ - PythonMutexPredicate().SetValue (LLDB_INVALID_THREAD_ID, eBroadcastAlways); -} - ScriptInterpreterPython::Locker::Locker (ScriptInterpreterPython *py_interpreter, uint16_t on_entry, uint16_t on_leave, FILE* wait_msg_handle) : m_need_session( (on_leave & TearDownSession) == TearDownSession ), - m_release_lock ( false ), // decide in constructor body m_python_interpreter(py_interpreter), m_tmp_fh(wait_msg_handle) { if (m_python_interpreter && !m_tmp_fh) m_tmp_fh = (m_python_interpreter->m_dbg_stdout ? m_python_interpreter->m_dbg_stdout : stdout); - - if ( (on_entry & AcquireLock) == AcquireLock ) - { - if (CurrentThreadHasPythonLock()) - { - if ( (on_leave & FreeLock) == FreeLock ) - m_release_lock = true; - } - else - { - DoAcquireLock(); - if ( (on_leave & FreeLock) == FreeLock ) - m_release_lock = true; - if ( (on_leave & FreeAcquiredLock) == FreeAcquiredLock ) - m_release_lock = true; - } - } + + DoAcquireLock(); if ( (on_entry & InitSession) == InitSession ) DoInitSession(); } @@ -191,13 +135,10 @@ ScriptInterpreterPython::Locker::Locker (ScriptInterpreterPython *py_interpreter bool ScriptInterpreterPython::Locker::DoAcquireLock() { - if (!CurrentThreadHasPythonLock()) - { - while (!TryGetPythonLock (1)) - if (m_tmp_fh) - fprintf (m_tmp_fh, - "Python interpreter locked on another thread; waiting to acquire lock...\n"); - } + LogSP log (lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_SCRIPT | LIBLLDB_LOG_VERBOSE)); + m_GILState = PyGILState_Ensure(); + if (log) + log->Printf("Ensured PyGILState. Previous state = %slocked\n", m_GILState == PyGILState_UNLOCKED ? "un" : ""); return true; } @@ -213,7 +154,10 @@ ScriptInterpreterPython::Locker::DoInitSession() bool ScriptInterpreterPython::Locker::DoFreeLock() { - ReleasePythonLock (); + LogSP log (lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_SCRIPT | LIBLLDB_LOG_VERBOSE)); + if (log) + log->Printf("Releasing PyGILState. Returning to state = %slocked\n", m_GILState == PyGILState_UNLOCKED ? "un" : ""); + PyGILState_Release(m_GILState); return true; } @@ -230,8 +174,7 @@ ScriptInterpreterPython::Locker::~Locker() { if (m_need_session) DoTearDownSession(); - if (m_release_lock) - DoFreeLock(); + DoFreeLock(); } class ForceDisableSyntheticChildren @@ -274,14 +217,15 @@ ScriptInterpreterPython::ScriptInterpreterPython (CommandInterpreter &interprete g_initialized = true; ScriptInterpreterPython::InitializePrivate (); } - - Locker locker(this, - ScriptInterpreterPython::Locker::AcquireLock, - ScriptInterpreterPython::Locker::FreeAcquiredLock); m_dictionary_name.append("_dict"); StreamString run_string; run_string.Printf ("%s = dict()", m_dictionary_name.c_str()); + + Locker locker(this, + ScriptInterpreterPython::Locker::AcquireLock, + ScriptInterpreterPython::Locker::FreeAcquiredLock); + PyRun_SimpleString (run_string.GetData()); run_string.Clear(); @@ -738,7 +682,13 @@ ScriptInterpreterPython::InputReaderCallback break; case eInputReaderDone: - script_interpreter->LeaveSession (); + { + Locker locker(script_interpreter, + ScriptInterpreterPython::Locker::AcquireLock, + ScriptInterpreterPython::Locker::FreeAcquiredLock); + + script_interpreter->LeaveSession (); + } // Restore terminal settings if they were validly saved if (log) @@ -1522,7 +1472,11 @@ ScriptInterpreterPython::RunEmbeddedPythonInterpreter (lldb::thread_arg_t baton) if (pty_slave_name != NULL) { StreamString run_string; - + // Ensure we have the GIL before running any Python code. (already done by the Locker object above) + // Since we're only running a few one-liners and then dropping to the interpreter + // (which will release the GIL when needed), we can just release the GIL after finishing our work. + // If finer-grained locking is desirable, we can lock and unlock the GIL only when calling a python function. + run_string.Printf ("run_one_line (%s, 'save_stderr = sys.stderr')", script_interpreter->m_dictionary_name.c_str()); PyRun_SimpleString (run_string.GetData()); run_string.Clear (); @@ -1542,36 +1496,25 @@ ScriptInterpreterPython::RunEmbeddedPythonInterpreter (lldb::thread_arg_t baton) // The following call drops into the embedded interpreter loop and stays there until the // user chooses to exit from the Python interpreter. + // This embedded interpreter will, as any Python code that performs I/O, unlock the GIL before performing + // a system call that can hang, and lock it when the syscall has returned. - // When in the embedded interpreter, the user can call arbitrary system and Python stuff, which may require - // the ability to run multi-threaded stuff, so we need to surround the call to the embedded interpreter with - // calls to Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS. - - // We ALSO need to surround the call to the embedded interpreter with calls to PyGILState_Ensure and - // PyGILState_Release. This is because this embedded interpreter is being run on a DIFFERENT THREAD than - // the thread on which the call to Py_Initialize (and PyEval_InitThreads) was called. Those initializations - // called PyGILState_Ensure on *that* thread, but it also needs to be called on *this* thread. Otherwise, - // if the user calls Python code that does threading stuff, the interpreter state will be off, and things could - // hang (it's happened before). + // We need to surround the call to the embedded interpreter with calls to PyGILState_Ensure and + // PyGILState_Release. This is because Python has a global lock which must be held whenever we want + // to touch any Python objects. Otherwise, if the user calls Python code, the interpreter state will be off, + // and things could go wrong (it's happened before). - Py_BEGIN_ALLOW_THREADS - PyGILState_STATE gstate = PyGILState_Ensure(); - run_string.Printf ("run_python_interpreter (%s)", script_interpreter->m_dictionary_name.c_str()); PyRun_SimpleString (run_string.GetData()); run_string.Clear (); - - PyGILState_Release (gstate); - Py_END_ALLOW_THREADS - + run_string.Printf ("run_one_line (%s, 'sys.stdin = save_stdin')", script_interpreter->m_dictionary_name.c_str()); PyRun_SimpleString (run_string.GetData()); run_string.Clear(); - + run_string.Printf ("run_one_line (%s, 'sys.stderr = save_stderr')", script_interpreter->m_dictionary_name.c_str()); PyRun_SimpleString (run_string.GetData()); run_string.Clear(); - } if (script_interpreter->m_embedded_thread_input_reader_sp) @@ -1727,8 +1670,6 @@ ScriptInterpreterPython::LoadScriptingModule (const char* pathname, lldb::DebuggerSP debugger_sp = m_interpreter.GetDebugger().shared_from_this(); { - Locker py_lock(this); - FileSpec target_file(pathname, true); // TODO: would we want to reject any other value? @@ -1784,7 +1725,9 @@ ScriptInterpreterPython::LoadScriptingModule (const char* pathname, error.SetErrorString("Python import statement failed"); return false; } - + + // Lock the GIL before calling into g_swig_call_module_init + Locker py_lock(this); // call __lldb_init_module(debugger,dict) if (!g_swig_call_module_init (basename, m_dictionary_name.c_str(), @@ -1920,7 +1863,18 @@ ScriptInterpreterPython::InitializePrivate () TerminalState stdin_tty_state; stdin_tty_state.Save(STDIN_FILENO, false); - PyEval_InitThreads (); + PyGILState_STATE gstate; + LogSP log (lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_SCRIPT | LIBLLDB_LOG_VERBOSE)); + bool threads_already_initialized = false; + if (PyEval_ThreadsInitialized()) { + gstate = PyGILState_Ensure(); + if (log) + log->Printf("Ensured PyGILState. Previous state = %slocked\n", gstate == PyGILState_UNLOCKED ? "un" : ""); + threads_already_initialized = true; + } else { + // InitThreads acquires the GIL if it hasn't been called before. + PyEval_InitThreads (); + } Py_InitializeEx (0); // Initialize SWIG after setting up python @@ -1969,6 +1923,15 @@ ScriptInterpreterPython::InitializePrivate () PyRun_SimpleString ("from embedded_interpreter import run_one_line"); PyRun_SimpleString ("from termios import *"); + // Release the GIL through the appropriate means. + if (threads_already_initialized) { + if (log) + log->Printf("Releasing PyGILState. Return to state = %slocked\n", gstate == PyGILState_UNLOCKED ? "un" : ""); + PyGILState_Release(gstate); + } else { + PyEval_SaveThread(); + } + stdin_tty_state.Restore(); }