[Lldb-commits] [lldb] r156200 - /lldb/trunk/source/Interpreter/ScriptInterpreterPython.cpp

[Filipe Cabecinhas]

That's just a small patch over an open wound. 
LLDB's usage of the Python GIL is wrong and only working because:
- In CLI usage and GUI usage no Python is being used except for the embedded interpreter
- Any Python usage is synchronous.

With an asynchronous, multithreaded Python program you can easily tell that LLDB will just happily execute Python code without having the GIL (if you enabled threads on SWIG), or hang (deadlocking) almost instantly (if you didn't enable threads on SWIG). 

I have written to the list in the past* and posted a patch to make LLDB use the GIL instead of the GIL + a home-made lock.
I'm attaching a revised version of the patch. If there's any problem or questions about the patch, please say so.

Thanks,

  Filipe

P.S: my previous message was this (http://lists.cs.uiuc.edu/pipermail/lldb-dev/2012-April/000886.html ):

Hi all,

As has been discussed here, the ScriptInterpreterPython class has its own locking, completely separate from the Python GIL. That won't work.

For GIL to work correctly, we have to release it when calling the lldb API from Python[1] (compile the swig definitions file with -threads and it will generate the appropriate calls), and lock it before calling any* Python function, in ScriptInterpreterPython. We also need to initialize the Python thread for the embedded interpreter thread that we create outside of Python-land[2]. 

The attached patch corrects ScriptInterpreterPython's locking mechanism to use the GIL, through the PyFILState_Ensure() and PyGILState_Release() functions. Each ScriptInterepreterPython::Locker object will lock the GIL in its constructor and unlock it in its destructor. The ScriptInterpreterPython initialization function also initializes the Python thread, if needed.

If we continue not using the GIL, any threaded Python program that uses lldb risks segfaulting at any time, due to lldb not locking the GIL and allowing other Python threads to run at the same time.

Regards,

  Filipe 


On Friday, May 4, 2012 at 9:37 PM, Johnny Chen wrote:

> Author: johnny
> Date: Fri May 4 15:37:11 2012
> New Revision: 156200
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=156200&view=rev
> Log:
> When the current thread state is NULL, PyThreadState_Get() issues a fatal error.
> Use a gentler API PyThreadState_GetDict(), instead.
> 
> rdar://problem/11292882
> 
> Modified:
> lldb/trunk/source/Interpreter/ScriptInterpreterPython.cpp
> 
> Modified: lldb/trunk/source/Interpreter/ScriptInterpreterPython.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Interpreter/ScriptInterpreterPython.cpp?rev=156200&r1=156199&r2=156200&view=diff
> ==============================================================================
> --- lldb/trunk/source/Interpreter/ScriptInterpreterPython.cpp (original)
> +++ lldb/trunk/source/Interpreter/ScriptInterpreterPython.cpp Fri May 4 15:37:11 2012
> @@ -386,7 +386,10 @@
> // in some (rare) cases during cleanup Python may end up believing we have no thread state
> // and PyImport_AddModule will crash if that is the case - since that seems to only happen
> // when destroying the SBDebugger, we can make do without clearing up stdout and stderr
> - if (PyThreadState_Get())
> +
> + // rdar://problem/11292882
> + // When the current thread state is NULL, PyThreadState_Get() issues a fatal error.
> + if (PyThreadState_GetDict())
> {
> PyObject *sysmod = PyImport_AddModule ("sys");
> PyObject *sysdict = PyModule_GetDict (sysmod);
> 
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu (mailto:lldb-commits at cs.uiuc.edu)
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits


-------------- next part --------------
A non-text attachment was scrubbed...
Name: lldb-use-GIL.patch
Type: application/octet-stream
Size: 13406 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20120505/d0c13473/attachment.obj>