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

[Zachary Turner]

I think you can reproduce these issues on MacOSX, you just need to build a
debug version of python yourself.  The difference is that on Windows, we
_have_ to build with a debug version of python.

I will try your suggested change of wrapping the calls to Release inside of
an if (!Py_IsInitialized()).

You might be right about the atexit handlers, and perhaps with the other
two issues fixed the atexit handlers can go back in.  Still, I'm not really
a fan of atexit handlers in general, and I think it makes for more readable
code if you can follow the sequence directly.  Maybe python atexit is more
lenient than C atexit handlers, which are notorious for restricting what
you can do inside them and perhaps why I'm wary of anything with the name
"atexit"


On Thu, Aug 7, 2014 at 5:39 PM, Enrico Granata <egranata at apple.com> wrote:

> 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/c1b9d0ea/attachment.html>