[pypy-commit] pypy default: In pystate.py, it's too late to call rffi.aroundstate: the immediate

[arigo]

Author: Armin Rigo <arigo at tunes.org>
Branch: 
Changeset: r76366:e8775e429f3d
Date: 2015-03-14 14:44 +0100
http://bitbucket.org/pypy/pypy/changeset/e8775e429f3d/

Log:	In pystate.py, it's too late to call rffi.aroundstate: the immediate
	caller, which is wrapper() function, needs to do it around
	everything. It fails an assert in debug builds and probably gives
	nonsense in non-debug builds.

diff --git a/pypy/module/cpyext/api.py b/pypy/module/cpyext/api.py
--- a/pypy/module/cpyext/api.py
+++ b/pypy/module/cpyext/api.py
@@ -192,7 +192,7 @@
 
 class ApiFunction:
     def __init__(self, argtypes, restype, callable, error=_NOT_SPECIFIED,
-                 c_name=None):
+                 c_name=None, gil=None):
         self.argtypes = argtypes
         self.restype = restype
         self.functype = lltype.Ptr(lltype.FuncType(argtypes, restype))
@@ -208,6 +208,7 @@
         assert argnames[0] == 'space'
         self.argnames = argnames[1:]
         assert len(self.argnames) == len(self.argtypes)
+        self.gil = gil
 
     def _freeze_(self):
         return True
@@ -223,14 +224,15 @@
     def get_wrapper(self, space):
         wrapper = getattr(self, '_wrapper', None)
         if wrapper is None:
-            wrapper = make_wrapper(space, self.callable)
+            wrapper = make_wrapper(space, self.callable, self.gil)
             self._wrapper = wrapper
             wrapper.relax_sig_check = True
             if self.c_name is not None:
                 wrapper.c_name = cpyext_namespace.uniquename(self.c_name)
         return wrapper
 
-def cpython_api(argtypes, restype, error=_NOT_SPECIFIED, external=True):
+def cpython_api(argtypes, restype, error=_NOT_SPECIFIED, external=True,
+                gil=None):
     """
     Declares a function to be exported.
     - `argtypes`, `restype` are lltypes and describe the function signature.
@@ -240,6 +242,8 @@
       SytemError.
     - set `external` to False to get a C function pointer, but not exported by
       the API headers.
+    - set `gil` to "acquire", "release" or "around" to acquire the GIL,
+      release the GIL, or both
     """
     if isinstance(restype, lltype.Typedef):
         real_restype = restype.OF
@@ -262,7 +266,8 @@
             c_name = None
         else:
             c_name = func_name
-        api_function = ApiFunction(argtypes, restype, func, error, c_name=c_name)
+        api_function = ApiFunction(argtypes, restype, func, error,
+                                   c_name=c_name, gil=gil)
         func.api_func = api_function
 
         if external:
@@ -594,12 +599,15 @@
 pypy_debug_catch_fatal_exception = rffi.llexternal('pypy_debug_catch_fatal_exception', [], lltype.Void)
 
 # Make the wrapper for the cases (1) and (2)
-def make_wrapper(space, callable):
+def make_wrapper(space, callable, gil=None):
     "NOT_RPYTHON"
     names = callable.api_func.argnames
     argtypes_enum_ui = unrolling_iterable(enumerate(zip(callable.api_func.argtypes,
         [name.startswith("w_") for name in names])))
     fatal_value = callable.api_func.restype._defl()
+    gil_acquire = (gil == "acquire" or gil == "around")
+    gil_release = (gil == "release" or gil == "around")
+    assert gil is None or gil_acquire or gil_release
 
     @specialize.ll()
     def wrapper(*args):
@@ -607,6 +615,10 @@
         from pypy.module.cpyext.pyobject import Reference
         # we hope that malloc removal removes the newtuple() that is
         # inserted exactly here by the varargs specializer
+        if gil_acquire:
+            after = rffi.aroundstate.after
+            if after:
+                after()
         rffi.stackcounter.stacks_counter += 1
         llop.gc_stack_bottom(lltype.Void)   # marker for trackgcroot.py
         retval = fatal_value
@@ -678,6 +690,10 @@
                 print str(e)
                 pypy_debug_catch_fatal_exception()
         rffi.stackcounter.stacks_counter -= 1
+        if gil_release:
+            before = rffi.aroundstate.before
+            if before:
+                before()
         return retval
     callable._always_inline_ = 'try'
     wrapper.__name__ = "wrapper for %r" % (callable, )
diff --git a/pypy/module/cpyext/pystate.py b/pypy/module/cpyext/pystate.py
--- a/pypy/module/cpyext/pystate.py
+++ b/pypy/module/cpyext/pystate.py
@@ -19,7 +19,7 @@
 class NoThreads(Exception):
     pass
 
- at cpython_api([], PyThreadState, error=CANNOT_FAIL)
+ at cpython_api([], PyThreadState, error=CANNOT_FAIL, gil="release")
 def PyEval_SaveThread(space):
     """Release the global interpreter lock (if it has been created and thread
     support is enabled) and reset the thread state to NULL, returning the
@@ -29,19 +29,15 @@
     state = space.fromcache(InterpreterState)
     tstate = state.swap_thread_state(
         space, lltype.nullptr(PyThreadState.TO))
-    if rffi.aroundstate.before:
-        rffi.aroundstate.before()
     return tstate
 
- at cpython_api([PyThreadState], lltype.Void)
+ at cpython_api([PyThreadState], lltype.Void, gil="acquire")
 def PyEval_RestoreThread(space, tstate):
     """Acquire the global interpreter lock (if it has been created and thread
     support is enabled) and set the thread state to tstate, which must not be
     NULL.  If the lock has been created, the current thread must not have
     acquired it, otherwise deadlock ensues.  (This function is available even
     when thread support is disabled at compile time.)"""
-    if rffi.aroundstate.after:
-        rffi.aroundstate.after()
     state = space.fromcache(InterpreterState)
     state.swap_thread_state(space, tstate)
 
@@ -182,17 +178,14 @@
     state = space.fromcache(InterpreterState)
     return state.swap_thread_state(space, tstate)
 
- at cpython_api([PyThreadState], lltype.Void)
+ at cpython_api([PyThreadState], lltype.Void, gil="acquire")
 def PyEval_AcquireThread(space, tstate):
     """Acquire the global interpreter lock and set the current thread state to
     tstate, which should not be NULL.  The lock must have been created earlier.
     If this thread already has the lock, deadlock ensues.  This function is not
     available when thread support is disabled at compile time."""
-    if rffi.aroundstate.after:
-        # After external call is before entering Python
-        rffi.aroundstate.after()
 
- at cpython_api([PyThreadState], lltype.Void)
+ at cpython_api([PyThreadState], lltype.Void, gil="release")
 def PyEval_ReleaseThread(space, tstate):
     """Reset the current thread state to NULL and release the global interpreter
     lock.  The lock must have been created earlier and must be held by the current
@@ -200,28 +193,20 @@
     that it represents the current thread state --- if it isn't, a fatal error is
     reported. This function is not available when thread support is disabled at
     compile time."""
-    if rffi.aroundstate.before:
-        # Before external call is after running Python
-        rffi.aroundstate.before()
 
 PyGILState_STATE = rffi.INT
 
- at cpython_api([], PyGILState_STATE, error=CANNOT_FAIL)
+ at cpython_api([], PyGILState_STATE, error=CANNOT_FAIL, gil="acquire")
 def PyGILState_Ensure(space):
     # XXX XXX XXX THIS IS A VERY MINIMAL IMPLEMENTATION THAT WILL HAPPILY
     # DEADLOCK IF CALLED TWICE ON THE SAME THREAD, OR CRASH IF CALLED IN A
     # NEW THREAD.  We should very carefully follow what CPython does instead.
-    if rffi.aroundstate.after:
-        # After external call is before entering Python
-        rffi.aroundstate.after()
     return rffi.cast(PyGILState_STATE, 0)
 
- at cpython_api([PyGILState_STATE], lltype.Void)
+ at cpython_api([PyGILState_STATE], lltype.Void, gil="release")
 def PyGILState_Release(space, state):
     # XXX XXX XXX We should very carefully follow what CPython does instead.
-    if rffi.aroundstate.before:
-        # Before external call is after running Python
-        rffi.aroundstate.before()
+    pass
 
 @cpython_api([], PyInterpreterState, error=CANNOT_FAIL)
 def PyInterpreterState_Head(space):
@@ -236,7 +221,8 @@
     """
     return lltype.nullptr(PyInterpreterState.TO)
 
- at cpython_api([PyInterpreterState], PyThreadState, error=CANNOT_FAIL)
+ at cpython_api([PyInterpreterState], PyThreadState, error=CANNOT_FAIL,
+             gil="around")
 def PyThreadState_New(space, interp):
     """Create a new thread state object belonging to the given interpreter
     object.  The global interpreter lock need not be held, but may be held if
@@ -245,12 +231,8 @@
         raise NoThreads
     # PyThreadState_Get will allocate a new execution context,
     # we need to protect gc and other globals with the GIL.
-    rffi.aroundstate.after()
-    try:
-        rthread.gc_thread_start()
-        return PyThreadState_Get(space)
-    finally:
-        rffi.aroundstate.before()
+    rthread.gc_thread_start()
+    return PyThreadState_Get(space)
 
 @cpython_api([PyThreadState], lltype.Void)
 def PyThreadState_Clear(space, tstate):
diff --git a/pypy/module/cpyext/test/test_translate.py b/pypy/module/cpyext/test/test_translate.py
--- a/pypy/module/cpyext/test/test_translate.py
+++ b/pypy/module/cpyext/test/test_translate.py
@@ -11,7 +11,7 @@
     FT = lltype.FuncType([], lltype.Signed)
     FTPTR = lltype.Ptr(FT)
 
-    def make_wrapper(space, func):
+    def make_wrapper(space, func, gil=None):
         def wrapper():
             return func(space)
         return wrapper