[Lldb-commits] [lldb-dev] Patches for python bindings

[Enrico Granata]

Hi

Replies inlined.

Enrico Granata
✉ egranata@.com
✆ (four oh eight) 862-7683

On Mar 14, 2012, at 4:15 PM, Filipe Cabecinhas wrote:

> Hi Enrico,
> 
> It doesn't necessarily depend on SWIG's thread features.  
> 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).
> 

It is correct to assume multithreading indeed.

> 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.

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.
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 :-)

> 
> 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.

It is true. You could arrange for LLDBSwigPythonCallPythonLogOutputCallback to manage mutual exclusion.
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?

> 
> If you would prefer to leave the threading issue to a later date, I can send a commit without those two lines, too.

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!

> 
> Regards,
> 
>  Filipe
> 
> 
> On Wednesday, March 14, 2012 at 8:42 PM, Enrico Granata wrote:
> 
>> Hi Filipe,
>> I have also looked at lldb-enable-log-callback.patch
>> 
>> 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).
>> 
>> 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?
>> 
>> Thanks,
>> Enrico Granata
>> ✉ egranata@.com
>> ✆ (408) 972-7683
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20120314/358bd3c3/attachment.html>