<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 7, 2014, at 7:01 PM, Zachary Turner <<a href="mailto:zturner@google.com" class="">zturner@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">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.<div class=""><br class=""></div></div></div></blockquote><div><br class=""></div><div>That’s a good idea, yes. Definitely worth a shot</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">
<div class="">I will try your suggested change of wrapping the calls to Release inside of an if (!Py_IsInitialized()).  </div></div></div></blockquote><div><br class=""></div><div>If we are going to go down this route, maybe the safe way to do this is rewrite Reset() such that:</div><div><br class=""></div><div>Reset (other = nullptr) {</div><div> if (Py_Initialized()) { decref(current) }</div><div> object = other</div><div> if (Py_Initialized()) { incref(object) }</div><div>}</div><div><br class=""></div><div>Then one could just say</div><div><br class=""></div><div>m_session_dict.Reset(nullptr)</div><div>m_sys_modules_dict.Reset(nullptr)</div><div><br class=""></div><div>If we are past a Py_Finalize(), then you will get the non-decref’ing behavior, otherwise the right thing will happen</div><div><br class=""></div><div>IMHO, it is much cleaner - and safer - than exposing a Release() API. I’d really really prefer to avoid such a backdoor<i class="">ish</i> API entirely, if at all feasible!</div><div>As an even stronger improvement, the destructor for PythonRefCountedObject might be moved from a plain decref() to a Reset(nullptr); then one wouldn’t even need the manual Reset() calls in the interpreter destructor - the destructors chain would just get it right all the time - which is A Good Thing</div><div>I am not sure if there was any reason to not do that (have the RefCountedObject destructor call Reset instead of manually decref'ing), so that might not be feasible after all, but worth a shot</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">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"</div>
</div><div class="gmail_extra"><br class=""></div></div></blockquote><div><br class=""></div><div>If atexit() is genuinely a cause of issues, sure we can stop using it. I guess my concern about this is not so much dotest.py per se - sure we can rework that - but I’d rather much not have limitations on what people can do when using the SB API - imagine a situation where someone is using LLDB’s API as well as some other Python library that chooses to use atexit() - if using LLDB’s API can’t coexist with atexit() now you’ve suddenly got a problem, something you have to document, and that can bite people in surprising ways</div><div>My preference here would be to fix the implementation of LLDB such that atexit() just works. A discussion on whether we’d like to use it in our test suite driver, or we prefer some other design, is orthogonal to that.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Aug 7, 2014 at 5:39 PM, Enrico Granata <span dir="ltr" class=""><<a href="mailto:egranata@apple.com" target="_blank" class="">egranata@apple.com</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class="">Hi there,</div><div class="">I have glanced over your patch</div><div class="">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</div>
<div class=""><br class=""></div><div class="">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</div>
<div class="">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</div>
<div class="">At the very least, would it be possible to wrap those releases in an if (Py_IsInitialized()) like you do in another change?</div><div class=""><br class=""></div><div class="">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</div>
<div class="">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</div>
<div class=""><br class=""></div><div class="">def exitFromTestSuite(exitCode = None):</div><div class=""> deleteCrashInfoDylib(dylib_dst)</div><div class=""> lldb.SBDebugger.Terminate()</div><div class=""> if exitCode != None:</div><div class="">    sys.exit(exitCode)</div><div class=""><br class="">
</div><div class="">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.</div><div class="">But, again, I am not sure why atexit is giving you woes - that would probably be worth looking into</div>
<div class=""><br class=""></div><br class=""><div class=""><blockquote type="cite" class=""><div class=""><div class="h5"><div class="">On Aug 7, 2014, at 4:03 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank" class="">zturner@google.com</a>> wrote:</div><br class=""></div>
</div><div class=""><div class=""><div class="h5">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:<br class="">
<br class="">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.<br class="">
<br class="">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()<br class="">
<br class="">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.<br class="">
<br class=""><a href="http://reviews.llvm.org/D4826" target="_blank" class="">http://reviews.llvm.org/D4826</a><br class=""><br class="">Files:<br class="">  include/lldb/Interpreter/PythonDataObjects.h<br class="">  source/Interpreter/ScriptInterpreterPython.cpp<br class="">  test/dotest.py<br class="">
</div></div><span class=""><D4826.12296.patch></span>_______________________________________________<br class="">lldb-commits mailing list<br class=""><a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank" class="">lldb-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br class="">
</div></blockquote></div><br class=""><div class="">
<div style="letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; word-wrap: break-word;" class=""><div style="letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; word-wrap: break-word;" class="">
<div style="letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; word-wrap: break-word;" class=""><div style="letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; word-wrap: break-word;" class="">
<div class=""><i class="">- Enrico</i><br class="">📩 egranata@<font color="#ff2600" class=""></font>.com ☎️ 27683</div><div class=""><br class=""></div></div></div></div></div><br class="">
</div>
<br class=""></div></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""><div class="">
<div style="color: rgb(0, 0, 0); letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div style="color: rgb(0, 0, 0); letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div style="color: rgb(0, 0, 0); letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div style="color: rgb(0, 0, 0); letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><i class="">- Enrico</i><br class="">📩 egranata@<font color="#ff2600" class=""></font>.com ☎️ 27683</div><div class=""><br class=""></div></div></div></div></div><br class="Apple-interchange-newline">
</div>
<br class=""></body></html>