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 <[email protected]> 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 <[email protected]> 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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits > > > *- Enrico* > 📩 egranata@.com ☎️ 27683 > > > >
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
