Author: Ralf Grosse-Kunstleve Date: 2022-01-17T10:32:19+01:00 New Revision: a6598575f4bc20f9a01c2bced2d0b1ff14d7576f
URL: https://github.com/llvm/llvm-project/commit/a6598575f4bc20f9a01c2bced2d0b1ff14d7576f DIFF: https://github.com/llvm/llvm-project/commit/a6598575f4bc20f9a01c2bced2d0b1ff14d7576f.diff LOG: [LLDB] Fix Python GIL-not-held issues The GIL must be held when calling any Python C API functions. In multithreaded applications that use callbacks this requirement can easily be violated by accident. A general tool to ensure GIL health is not available, but patching Python Py_INCREF to add an assert provides a basic health check: ``` +int PyGILState_Check(void); /* Include/internal/pystate.h */ + #define Py_INCREF(op) ( \ + assert(PyGILState_Check()), \ _Py_INC_REFTOTAL _Py_REF_DEBUG_COMMA \ ((PyObject *)(op))->ob_refcnt++) #define Py_DECREF(op) \ do { \ + assert(PyGILState_Check()); \ PyObject *_py_decref_tmp = (PyObject *)(op); \ if (_Py_DEC_REFTOTAL _Py_REF_DEBUG_COMMA \ --(_py_decref_tmp)->ob_refcnt != 0) \ ``` Adding this assertion causes around 50 test failures in LLDB. Adjusting the scope of things guarded by `py_lock` fixes them. More background: https://docs.python.org/3/glossary.html#term-global-interpreter-lock Patch by Ralf Grosse-Kunstleve Differential Revision: https://reviews.llvm.org/D114722 Added: Modified: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp Removed: ################################################################################ diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp index 7c71c9329e579..d68af672ae83e 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp @@ -257,6 +257,7 @@ PythonObject PythonObject::GetAttributeValue(llvm::StringRef attr) const { } StructuredData::ObjectSP PythonObject::CreateStructuredObject() const { + assert(PyGILState_Check()); switch (GetObjectType()) { case PyObjectType::Dictionary: return PythonDictionary(PyRefType::Borrowed, m_py_obj) diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h index 56bc55d239d12..9d2cdca45a633 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h @@ -88,12 +88,21 @@ class StructuredPythonObject : public StructuredData::Generic { StructuredPythonObject() : StructuredData::Generic() {} StructuredPythonObject(void *obj) : StructuredData::Generic(obj) { + assert(PyGILState_Check()); Py_XINCREF(GetValue()); } ~StructuredPythonObject() override { - if (Py_IsInitialized()) - Py_XDECREF(GetValue()); + if (Py_IsInitialized()) { + if (_Py_IsFinalizing()) { + // Leak GetValue() rather than crashing the process. + // https://docs.python.org/3/c-api/init.html#c.PyGILState_Ensure + } else { + PyGILState_STATE state = PyGILState_Ensure(); + Py_XDECREF(GetValue()); + PyGILState_Release(state); + } + } SetValue(nullptr); } @@ -264,8 +273,16 @@ class PythonObject { ~PythonObject() { Reset(); } void Reset() { - if (m_py_obj && Py_IsInitialized()) - Py_DECREF(m_py_obj); + if (m_py_obj && Py_IsInitialized()) { + if (_Py_IsFinalizing()) { + // Leak m_py_obj rather than crashing the process. + // https://docs.python.org/3/c-api/init.html#c.PyGILState_Ensure + } else { + PyGILState_STATE state = PyGILState_Ensure(); + Py_DECREF(m_py_obj); + PyGILState_Release(state); + } + } m_py_obj = nullptr; } diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp index ed4ad279f5281..96725afd279ed 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp @@ -1438,14 +1438,9 @@ ScriptInterpreterPythonImpl::CreateFrameRecognizer(const char *class_name) { if (class_name == nullptr || class_name[0] == '\0') return StructuredData::GenericSP(); - void *ret_val; - - { - Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, - Locker::FreeLock); - ret_val = LLDBSWIGPython_CreateFrameRecognizer(class_name, - m_dictionary_name.c_str()); - } + Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock); + void *ret_val = LLDBSWIGPython_CreateFrameRecognizer( + class_name, m_dictionary_name.c_str()); return StructuredData::GenericSP(new StructuredPythonObject(ret_val)); } @@ -1502,14 +1497,9 @@ ScriptInterpreterPythonImpl::OSPlugin_CreatePluginObject( if (!process_sp) return StructuredData::GenericSP(); - void *ret_val; - - { - Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, - Locker::FreeLock); - ret_val = LLDBSWIGPythonCreateOSPlugin( - class_name, m_dictionary_name.c_str(), process_sp); - } + Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock); + void *ret_val = LLDBSWIGPythonCreateOSPlugin( + class_name, m_dictionary_name.c_str(), process_sp); return StructuredData::GenericSP(new StructuredPythonObject(ret_val)); } @@ -1757,17 +1747,13 @@ StructuredData::ObjectSP ScriptInterpreterPythonImpl::CreateScriptedThreadPlan( if (!python_interpreter) return {}; - void *ret_val; - - { - Locker py_lock(this, - Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); - ret_val = LLDBSwigPythonCreateScriptedThreadPlan( - class_name, python_interpreter->m_dictionary_name.c_str(), - args_data, error_str, thread_plan_sp); - if (!ret_val) - return {}; - } + Locker py_lock(this, + Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); + void *ret_val = LLDBSwigPythonCreateScriptedThreadPlan( + class_name, python_interpreter->m_dictionary_name.c_str(), args_data, + error_str, thread_plan_sp); + if (!ret_val) + return {}; return StructuredData::ObjectSP(new StructuredPythonObject(ret_val)); } @@ -1860,16 +1846,12 @@ ScriptInterpreterPythonImpl::CreateScriptedBreakpointResolver( if (!python_interpreter) return StructuredData::GenericSP(); - void *ret_val; - - { - Locker py_lock(this, - Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); + Locker py_lock(this, + Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); - ret_val = LLDBSwigPythonCreateScriptedBreakpointResolver( - class_name, python_interpreter->m_dictionary_name.c_str(), args_data, - bkpt_sp); - } + void *ret_val = LLDBSwigPythonCreateScriptedBreakpointResolver( + class_name, python_interpreter->m_dictionary_name.c_str(), args_data, + bkpt_sp); return StructuredData::GenericSP(new StructuredPythonObject(ret_val)); } @@ -1935,16 +1917,12 @@ StructuredData::GenericSP ScriptInterpreterPythonImpl::CreateScriptedStopHook( return StructuredData::GenericSP(); } - void *ret_val; - - { - Locker py_lock(this, - Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); + Locker py_lock(this, + Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); - ret_val = LLDBSwigPythonCreateScriptedStopHook( - target_sp, class_name, python_interpreter->m_dictionary_name.c_str(), - args_data, error); - } + void *ret_val = LLDBSwigPythonCreateScriptedStopHook( + target_sp, class_name, python_interpreter->m_dictionary_name.c_str(), + args_data, error); return StructuredData::GenericSP(new StructuredPythonObject(ret_val)); } @@ -2035,14 +2013,10 @@ ScriptInterpreterPythonImpl::CreateSyntheticScriptedProvider( if (!python_interpreter) return StructuredData::ObjectSP(); - void *ret_val = nullptr; - - { - Locker py_lock(this, - Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); - ret_val = LLDBSwigPythonCreateSyntheticProvider( - class_name, python_interpreter->m_dictionary_name.c_str(), valobj); - } + Locker py_lock(this, + Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); + void *ret_val = LLDBSwigPythonCreateSyntheticProvider( + class_name, python_interpreter->m_dictionary_name.c_str(), valobj); return StructuredData::ObjectSP(new StructuredPythonObject(ret_val)); } @@ -2057,14 +2031,10 @@ ScriptInterpreterPythonImpl::CreateScriptCommandObject(const char *class_name) { if (!debugger_sp.get()) return StructuredData::GenericSP(); - void *ret_val; - - { - Locker py_lock(this, - Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); - ret_val = LLDBSwigPythonCreateCommandObject( - class_name, m_dictionary_name.c_str(), debugger_sp); - } + Locker py_lock(this, + Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); + void *ret_val = LLDBSwigPythonCreateCommandObject( + class_name, m_dictionary_name.c_str(), debugger_sp); return StructuredData::GenericSP(new StructuredPythonObject(ret_val)); } @@ -2176,8 +2146,11 @@ bool ScriptInterpreterPythonImpl::GetScriptedSummary( return false; } - if (new_callee && old_callee != new_callee) + if (new_callee && old_callee != new_callee) { + Locker py_lock(this, + Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN); callee_wrapper_sp = std::make_shared<StructuredPythonObject>(new_callee); + } return ret_val; } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits