[lldb] Eliminate (_)Py_IsFinalizing (NFC)

[JDevlieghere]

Looking at the implementation of pylifecycle.c in cpython, finalizing and initialized are set at the same time. Therefore we can eliminate the call to Py_IsFinalizing and only check Py_IsInitialized, which is part of the stable API.

I converted the check to an assert and confirmed that during my test suite runs, we never got into the if block. Because we check before taking the lock, there is an opportunity for a race, but that exact same race exists with the original code.

[JDevlieghere]

I got the inspiration from https://github.com/pytorch/pytorch/blob/589072afa1a196270b0298460c1f419e209da6ea/torch/csrc/utils/pybind.h#L178-L185

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Looking at the implementation of pylifecycle.c in cpython, finalizing and initialized are set at the same time. Therefore we can eliminate the call to Py_IsFinalizing and only check Py_IsInitialized, which is part of the stable API.

I converted the check to an assert and confirmed that during my test suite runs, we never got into the if block. Because we check before taking the lock, there is an opportunity for a race, but that exact same race exists with the original code.

---

Full diff: https://github.com/llvm/llvm-project/pull/152226.diff

1 Files Affected:

• (modified) lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp (+3-16)

diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
index 82aa022bbae0b..27ac54322165e 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -71,24 +71,11 @@ Expected<std::string> python::As<std::string>(Expected<PythonObject> &&obj) {
   return std::string(utf8.get());
 }
 
-static bool python_is_finalizing() {
-#if PY_VERSION_HEX >= 0x030d0000
-  return Py_IsFinalizing();
-#else
-  return _Py_IsFinalizing();
-#endif
-}
-
 void PythonObject::Reset() {
   if (m_py_obj && Py_IsInitialized()) {
-    if (python_is_finalizing()) {
-      // 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);
-    }
+    PyGILState_STATE state = PyGILState_Ensure();
+    Py_DECREF(m_py_obj);
+    PyGILState_Release(state);
   }
   m_py_obj = nullptr;
 }

[labath]

Makes sense. I gave this a spin on our internal build, and it seems to work fine.