[Lldb-commits] [PATCH] Fixup some issues with Python shutdown / finalization

[Enrico Granata]

Hi there,
I have glanced over your patch
I do not have a Windows box to try and repro these shutdown issues, so I might very well be missing some critical Windows-specific detail in my feedback

Release() seems to be a fairly dangerous accessor - indeed I am not positive that releasing without deallocation the session and modules dictionaries will do the right thing when Py_Finalize() is not involved
I might very well create an SBDebugger (which - via the command interpreter - owns the ScriptInterpreterPython); if I simply let go of the last reference to the SBDebugger, the destruction chain should do the right thing, and I expect your release-without-decref calls might cause some objects to survive their due lifetime
At the very least, would it be possible to wrap those releases in an if (Py_IsInitialized()) like you do in another change?

As for 3 - nothing in the Python documentation specifies that the atexit facility does not allow any “meaningful code” execution; the main caveat I could find online about it is that there are escape routes out of Python that atexit() does not handle - but that seems a very different concern from what you are seeing
Even if we actually end up deciding that atexit is not to be used, your patch as-is has eliminated the call to the code to delete crashinfo.so - that is not critical of course, but it worries me to see a duplication of lldb.SBDebugger.Terminate() calls - it might be more appropriate, if anything, to define a function

def exitFromTestSuite(exitCode = None):
 deleteCrashInfoDylib(dylib_dst)
 lldb.SBDebugger.Terminate()
 if exitCode != None:
    sys.exit(exitCode)

Then we would all know to put our atexit calls in exitFromTestSuite(), and we’d have some degree of confidence that all cleanup tasks will be executed on all ways out of the test suite.
But, again, I am not sure why atexit is giving you woes - that would probably be worth looking into


> On Aug 7, 2014, at 4:03 PM, Zachary Turner <zturner at google.com> wrote:
> 
> There were a number of issues related to python shutdown.  These issues appear to be present on every platform, but are benign except for Windows.  On Windows they present because Windows must build against a debug python interpreter, which asserts when these issues arise.  The issues, along with their respective fixes, are:
> 
> 1) decrementing the reference count of dead objects.  This happens because we store the session dictionary in the ScriptInterpreterPython class, and when these are destroyed on shutdown, they get decref'ed.  However, this happens during or after a Py_Finalize, so Python has already destroyed these objects.  The fix employed here is to add a Release() method to PythonObject() which simply drops the underlying object.
> 
> 2) Attempting to execute embedded python during Py_Finalize.  This results in error messages "NameError: module 'lldb' not found" after tests run.  The fix here is to only execute these commands if Py_IsInitialized()
> 
> 3) Use of sys.atexit().  atexit handlers are run as part of Py_Finalize, and at this point it is unspecified which, if any modules have been destroyed.  So no meaningful code can run here.  The fix employed here is to explicitly call lldb.SBDebugger.Terminate() before calling sys.exit().  A better fix would be to not use sys.exit() at all, but instead have a clean shutdown path, perhaps by way of raising an exception that is caught at the top-level of the python script.
> 
> http://reviews.llvm.org/D4826
> 
> Files:
>  include/lldb/Interpreter/PythonDataObjects.h
>  source/Interpreter/ScriptInterpreterPython.cpp
>  test/dotest.py
> <D4826.12296.patch>_______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits

- Enrico
📩 egranata@.com ☎️ 27683



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140807/852c5714/attachment.html>