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

Wed Jan 12 11:28:26 PST 2022

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

LGTM

================
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);
----------------
labath wrote:
> labath wrote:
> > 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.
> 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.
Makes sense!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114722