<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Filipe,<div>replies inlined.</div><div><br><div>
<span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><span class="Apple-style-span" style="orphans: 2; text-align: -webkit-auto; text-indent: 0px; widows: 2; -webkit-text-decorations-in-effect: none; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; color: rgb(0, 0, 0); "><i>Enrico Granata</i></div><div style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; ">✉ egranata@<font class="Apple-style-span" color="#ff230e"></font>.com</div><div>✆ (four oh eight) 862-7683</div></div></span></div></span></div></span></div></span></div></span></span>
</div>
<br><div><div>On Mar 15, 2012, at 2:40 PM, Filipe Cabecinhas wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Hi Enrico, <br><br>Well, for now I can't, using the public API, lock the ScriptPythonIntepreter lock. </div></blockquote><div><br></div><div>True.</div><br><blockquote type="cite"><div>I will change that class and make it use only the Python-blessed GIL locking mechanism, instead of the home-made lock. Otherwise the Python bindings will only serve for synchronous debugging, since other Python threads can never lock it.<br></div></blockquote><div><br></div><div>This might be the right thing to do, or it might not.</div><div><br></div><div>Having multiple locking mechanisms and some code uses one and some uses another is a no-no, since that amounts to having no locking at all :-) We need to pick one way. If the GIL approach has merits (other than being The Dogmatic Right Way), then I guess one might implement the Locker class to use Python's GIL macros.</div><div><br></div><div>You would rewrite:</div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> <span style="color: #b933a1">bool</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> DoAcquireLock ();</div><p style="margin: 0.0px 0.0px 0.0px 0.0px; font: 11.0px Menlo; min-height: 13.0px"> <br class="webkit-block-placeholder"></p><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> <span style="color: #b933a1">bool</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> DoFreeLock ();</div><div>to use the GIL macros. The rest of the code should be unaffected.</div><div><br></div><div>However, in the future we could need to keep track of additional information for the lock. Currently, all we store is a thread-id, but being able to store additional information (e.g. which method acquired the lock) might become important. If you do a major rewrite of the locking code, you should keep this in mind as a requirement.</div></div><div><div><br></div><div>An alternative approach would be for your log callback function to take a struct as a baton, and have it contain the actual PyObject* wrapping the function plus a ScriptInterpreterPython*. Then you would have your SWIG-binding call routed through the ScriptInterpreter (much like it happens for data formatters today). This would enable you to merge seamlessly in the way locks are currently managed. Even better, we now have a ScriptInterpreterObject class that efficiently does the right thing when it comes to ref-counting scripted objects. If you feel the need to pass objects from Python to C++ and back, consider wrapping the PyObject*. All you need to call is <span class="Apple-style-span" style="color: rgb(80, 129, 135); font-family: Menlo; font-size: 11px; ">ScriptInterpreterPython</span><span class="Apple-style-span" style="color: rgb(80, 129, 135); font-family: Menlo; font-size: 11px; "><span style="color: #000000">::MakeScriptObject </span></span>and you get a C++ shared pointer to the object in return.</div></div><br><blockquote type="cite"><div><br>Regards, <br><br> Filipe<br><br><br>On Thursday, March 15, 2012 at 1:02 AM, Enrico Granata wrote:<br><br><blockquote type="cite">Hi<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Replies inlined.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Enrico Granata<br></blockquote><blockquote type="cite">✉ egranata@.com<br></blockquote><blockquote type="cite">✆ (four oh eight) 862-7683<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">On Mar 14, 2012, at 4:15 PM, Filipe Cabecinhas wrote:<br></blockquote><blockquote type="cite"><blockquote type="cite">Hi Enrico,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">It doesn't necessarily depend on SWIG's thread features. <br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">The thing is: if we're in a multithreading python environment (which is what we have to assume, when there is a python program using lldb's event API), then the callback helper function will lock the GIL before calling python code. If SWIG isn't run with the "-threads" argument, then those calls are no-ops (SWIG_PYTHON_THREADS is undefined and those two symbols will be defined to nothing).<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">It is correct to assume multithreading indeed.<br></blockquote><blockquote type="cite"><blockquote type="cite">I'm locking the GIL because I'm planning on fixing lldb's ScriptInterpreterPython code to lock the GIL when it needs to. When that happens, these locks will need to be here to make the callback feature continue working.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">We are currently using our own Locker implementation. If you find any bugs with it, please feel free to let us know/fix them and send in a patch for review.<br></blockquote><blockquote type="cite">If there are good reasons to move from what we are doing to the GIL locking, and nothing get broken in the conversion, then again send in a patch :-)<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">There's also no problem in having several threads calling LLDBSwigPythonCallPythonLogOutputCallback at the same time: the callback would be the one responsible for any races with output channels, since LLDB doesn't know anything about what will happen to the strings in the callback function.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">It is true. You could arrange for LLDBSwigPythonCallPythonLogOutputCallback to manage mutual exclusion.<br></blockquote><blockquote type="cite">My question is how are you going to make sure that the locking is synchronized with what ScriptInterpreterPython does, so that don't end up having a log-output callback and something else entirely be running into Python *together*, both believing they hold an exclusive lock?<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">If you would prefer to leave the threading issue to a later date, I can send a commit without those two lines, too.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I would definitely address the threading issue in a separate and comprehensive patch. Moving to the GIL locking might be the right thing to do, but having most of the code use the Locker class and a few places the GIL locking seems a good call for trouble!<br></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Regards,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Filipe<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">On Wednesday, March 14, 2012 at 8:42 PM, Enrico Granata wrote:<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">Hi Filipe,<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">I have also looked at lldb-enable-log-callback.patch<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">As far as I can tell, it depends on SWIG's threading support for its operation (since otherwise it looks like nothing prevents multiple threads from calling LLDBSwigPythonCallPythonLogOutputCallback).<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">Is there a way to change the patch so it does not depend on that feature, since apparently it is not working well with the existing LLDB code?<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">Thanks,<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">Enrico Granata<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">✉ egranata@.com<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">✆ (408) 972-7683<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><br></blockquote><br><br><br></div></blockquote></div><br></div></body></html>