[Lldb-commits] [PATCH] D114722: [LLDB] Fix Python GIL-not-held issues

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: JDevlieghere.
labath added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:101
+      } else {
+        auto gstate = PyGILState_Ensure();
+        Py_XDECREF(GetValue());
----------------
This usage of auto isn't very llvm-ish.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:277-285
+      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 {
+        auto gstate = PyGILState_Ensure();
+        Py_DECREF(m_py_obj);
+        PyGILState_Release(gstate);
----------------
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.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:1566
 
   {
     Locker py_lock(this, Locker::AcquireLock | Locker::NoSTDIN,
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114722/new/

https://reviews.llvm.org/D114722