[LLDB] Fix Python GIL-not-held issues

[labath]

Summary

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

[labath]

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

101

This usage of auto isn't very llvm-ish.

277–285

As I mentioned internally, I think this should be placed inside the ScriptInterpreterPythonImpl destructor (and elsewhere, if needed), as that's the level at which we do our normal locking. There it can become a regular Locker object, since that function will be called from SBDebugger::Terminate (or maybe even earlier, in either case it will be before any global destructors run).

The reason this can't be done with StructuredPythonObject, is because this one gets passed on to python code, which can delete it (or not) at pretty much arbitrary time. PythonObjects OTOH, are just C++ handles to python objects, and we should be able to keep track of their lifetimes reasonably well.

We can put an assert here to ensure the callers obey this contract.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

1415–1416

Since the only reason this scope was introduced was to run the return statement in an unlocked state, I think it would be better to just remove it now. Same goes for other patterns like this.

[rupprecht]

Rebase

[labath]

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

277–285

It turns out this is not as easy as I made it sound. I thought one could just put the lock around the body of the destructor, but (of course) destructors for subobjects run only after the body of the main object desctructor terminates. I mean, it would still be possible to make it work that way, but it wouldn't be as pretty. So, after some soul-searching, I decided to keep the current implementation.

[labath]

This just fixes the style issues.

[JDevlieghere]

LGTM

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

277–285

Makes sense!

[sylvestre.ledru]

Seems that it broke apt.llvm.org on ubuntu bionic with Python 3.6:
I reported this issue: https://github.com/llvm/llvm-project/issues/53291

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

protected:
   PyGILState_STATE m_state;
 };
 
 class StructuredPythonObject : public StructuredData::Generic {
 public:
   StructuredPythonObject() : StructuredData::Generic() {}
 
   StructuredPythonObject(void *obj) : StructuredData::Generic(obj) {
+    assert(PyGILState_Check());
     Py_XINCREF(GetValue());
   }
 
   ~StructuredPythonObject() override {
-    if (Py_IsInitialized())
+    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);
   }
 
   bool IsValid() const override { return GetValue() && GetValue() != Py_None; }
 
   void Serialize(llvm::json::OStream &s) const override;
 
 private:
public:
   PythonObject(PythonObject &&rhs) {
     m_py_obj = rhs.m_py_obj;
     rhs.m_py_obj = nullptr;
   }
 
   ~PythonObject() { Reset(); }
 
   void Reset() {
-    if (m_py_obj && Py_IsInitialized())
+    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;
   }
 
   void Dump() const {
     if (m_py_obj)
       _PyObject_Dump(m_py_obj);
     else
       puts("NULL");

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

PythonObject PythonObject::GetAttributeValue(llvm::StringRef attr) const {
   if (!PyObject_HasAttr(m_py_obj, py_attr.get()))
     return PythonObject();
 
   return PythonObject(PyRefType::Owned,
                       PyObject_GetAttr(m_py_obj, py_attr.get()));
 }
 
 StructuredData::ObjectSP PythonObject::CreateStructuredObject() const {
+  assert(PyGILState_Check());
   switch (GetObjectType()) {
   case PyObjectType::Dictionary:
     return PythonDictionary(PyRefType::Borrowed, m_py_obj)
         .CreateStructuredDictionary();
   case PyObjectType::Boolean:
     return PythonBoolean(PyRefType::Borrowed, m_py_obj)
         .CreateStructuredBoolean();
   case PyObjectType::Integer:

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

bool ScriptInterpreterPythonImpl::GenerateTypeSynthClass(
   output.assign(auto_generated_class_name);
   return true;
 }
 
 StructuredData::GenericSP
 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));
 }
 
 lldb::ValueObjectListSP ScriptInterpreterPythonImpl::GetRecognizedArguments(
     const StructuredData::ObjectSP &os_plugin_object_sp,
     lldb::StackFrameSP frame_sp) {
   Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);

 ScriptInterpreterPythonImpl::OSPlugin_CreatePluginObject(
     const char *class_name, lldb::ProcessSP process_sp) {
   if (class_name == nullptr || class_name[0] == '\0')
     return StructuredData::GenericSP();
 
   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(
 
   return StructuredData::GenericSP(new StructuredPythonObject(ret_val));
 }
 
 StructuredData::DictionarySP ScriptInterpreterPythonImpl::OSPlugin_RegisterInfo(
     StructuredData::ObjectSP os_plugin_object_sp) {
   Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN, Locker::FreeLock);
 
StructuredData::ObjectSP ScriptInterpreterPythonImpl::CreateScriptedThreadPlan(
 
   Debugger &debugger = thread_plan_sp->GetTarget().GetDebugger();
   ScriptInterpreterPythonImpl *python_interpreter =
       GetPythonInterpreter(debugger);
 
   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 {};
-  }
+  void *ret_val = LLDBSwigPythonCreateScriptedThreadPlan(
+      class_name, python_interpreter->m_dictionary_name.c_str(), args_data,
+      error_str, thread_plan_sp);
 
   return StructuredData::ObjectSP(new StructuredPythonObject(ret_val));
 }
 
 bool ScriptInterpreterPythonImpl::ScriptedThreadPlanExplainsStop(
     StructuredData::ObjectSP implementor_sp, Event *event, bool &script_error) {
   bool explains_stop = true;
   StructuredData::Generic *generic = nullptr;
ScriptInterpreterPythonImpl::CreateScriptedBreakpointResolver(
 
   Debugger &debugger = bkpt_sp->GetTarget().GetDebugger();
   ScriptInterpreterPythonImpl *python_interpreter =
       GetPythonInterpreter(debugger);
 
   if (!python_interpreter)
     return StructuredData::GenericSP();
 
-  void *ret_val;
-
-  {
-    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(
 
   return StructuredData::GenericSP(new StructuredPythonObject(ret_val));
 }
 
 bool ScriptInterpreterPythonImpl::ScriptedBreakpointResolverSearchCallback(
     StructuredData::GenericSP implementor_sp, SymbolContext *sym_ctx) {
   bool should_continue = false;
 
StructuredData::GenericSP ScriptInterpreterPythonImpl::CreateScriptedStopHook(
   ScriptInterpreterPythonImpl *python_interpreter =
       GetPythonInterpreter(m_debugger);
 
   if (!python_interpreter) {
     error.SetErrorString("No script interpreter for scripted stop-hook.");
     return StructuredData::GenericSP();
   }
 
-  void *ret_val;
-
-  {
-    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(
 
   return StructuredData::GenericSP(new StructuredPythonObject(ret_val));
 }
 
 bool ScriptInterpreterPythonImpl::ScriptedStopHookHandleStop(
     StructuredData::GenericSP implementor_sp, ExecutionContext &exc_ctx,
     lldb::StreamSP stream_sp) {
   assert(implementor_sp &&
ScriptInterpreterPythonImpl::CreateSyntheticScriptedProvider(
 
   Debugger &debugger = target->GetDebugger();
   ScriptInterpreterPythonImpl *python_interpreter =
       GetPythonInterpreter(debugger);
 
   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);
-  }
+  void *ret_val = LLDBSwigPythonCreateSyntheticProvider(
 
   return StructuredData::ObjectSP(new StructuredPythonObject(ret_val));
 }
 
 StructuredData::GenericSP
 ScriptInterpreterPythonImpl::CreateScriptCommandObject(const char *class_name) {
   DebuggerSP debugger_sp(m_debugger.shared_from_this());
 
   if (class_name == nullptr || class_name[0] == '\0')
     return StructuredData::GenericSP();
 
   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);
-  }
+  void *ret_val = LLDBSwigPythonCreateCommandObject(
 
   return StructuredData::GenericSP(new StructuredPythonObject(ret_val));
 }
 
 bool ScriptInterpreterPythonImpl::GenerateTypeScriptFunction(
     const char *oneliner, std::string &output, const void *name_token) {
   StringList input;
   input.SplitIntoLines(oneliner, strlen(oneliner));
if (python_function_name && *python_function_name) {
             &new_callee, options_sp, retval);
       }
     }
   } else {
     retval.assign("<no function name>");
     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;
 }
 
 bool ScriptInterpreterPythonImpl::BreakpointCallbackFunction(
     void *baton, StoppointCallbackContext *context, user_id_t break_id,
     user_id_t break_loc_id) {
   CommandDataPython *bp_option_data = (CommandDataPython *)baton;