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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
