Skip to content

Commit

Permalink
[LLDB] Fix Python GIL-not-held issues
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@rwgk @labath
rwgk authored and labath committed Jan 17, 2022
1 parent af12a3f commit a659857
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 66 deletions.
1 change: 1 addition & 0 deletions lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
View file
Expand Up @@ -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)
Expand Down
25 changes: 21 additions & 4 deletions lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
View file
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
}

Expand Down
97 changes: 35 additions & 62 deletions lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
View file
Expand Up @@ -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));
}
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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));
}
Expand All @@ -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));
}
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit a659857

Please sign in to comment.