Cython/Compiler/ExprNodes.py

@@ -737,6 +737,11 @@ class ExprNode(Node):
         #  a subnode.
         return self.is_temp
 
+    def result_is_new_reference(self):
+        # Return true if the result is a new reference that is
+        # already incref-ed and will need to be decref-ed later.
+        return self.result_in_temp()
+
     def target_code(self):
         #  Return code fragment for use as LHS of a C assignment.
         return self.calculate_result_code()
@@ -786,7 +791,7 @@ class ExprNode(Node):
         Make sure we own a reference to result.
         If the result is in a temp, it is already a new reference.
         """
-        if not self.result_in_temp():
+        if not self.result_is_new_reference():
             # FIXME: is this verification really necessary ?
             if self.type.is_cyp_class and "NULL" in self.result():
                 pass
@@ -798,7 +803,7 @@ class ExprNode(Node):
         Make sure we own the reference to this memoryview slice.
         """
         # TODO ideally this would be shared with "make_owned_reference"
-        if not self.result_in_temp():
+        if not self.result_is_new_reference():
             code.put_incref_memoryviewslice(self.result(), self.type,
                                             have_gil=not self.in_nogil_context)
 
@@ -3765,7 +3770,14 @@ class IndexNode(_IndexingBaseNode):
             return py_object_type
 
     def analyse_types(self, env):
-        return self.analyse_base_and_index_types(env, getting=True)
+        index_node = self.analyse_base_and_index_types(env, getting=True)
+        if index_node.type.is_cyp_class:
+            # If a[b] is a cypclass, a new reference is returned.
+            # In that case it must be stored for later decref-ing.
+            # XXX: future optimisation: only coerce to temporary
+            # when the value is not already assigned to a variable.
+            return index_node.coerce_to_temp(env)
+        return index_node
 
     def analyse_target_types(self, env):
         node = self.analyse_base_and_index_types(env, setting=True)
@@ -4234,13 +4246,13 @@ class IndexNode(_IndexingBaseNode):
             temp.use_managed_ref = False
         return temp
 
-    def make_owned_reference(self, code):
+    def result_is_new_reference(self):
         if self.type.is_cyp_class and not (self.base.type.is_array or self.base.type.is_ptr):
             # This is already a new reference
             # either via cpp operator[]
             # or via cypclass __getitem__
-            return
-        ExprNode.make_owned_reference(self, code)
+            return True
+        return ExprNode.result_is_new_reference(self)
 
     gil_message = "Indexing Python object"
 
@@ -7295,6 +7307,7 @@ class AttributeNode(ExprNode):
         return node
 
     def analyse_types(self, env, target = 0):
+        self.is_target = target
         self.initialized_check = env.directives['initializedcheck']
         node = self.analyse_as_cimported_attribute_node(env, target)
         if node is None and not target:
@@ -8025,7 +8038,7 @@ class SequenceNode(ExprNode):
 
             for i in range(arg_count):
                 arg = self.args[i]
-                if c_mult or not arg.result_in_temp():
+                if c_mult or not arg.result_is_new_reference():
                     code.put_incref(arg.result(), arg.ctype())
                 arg.generate_giveref(code)
                 code.putln("%s(%s, %s, %s);" % (
@@ -11517,7 +11530,7 @@ class BinopNode(ExprNode):
     #      - Determine result type and result code fragment.
     #      - Allocate temporary for result if needed.
 
-    subexprs = ['operand1', 'operand2']
+    subexprs = ['operand2', 'operand1']
     inplace = False
     op_func_type = None
 
@@ -14162,17 +14175,27 @@ class CoerceToLockedNode(CoercionNode):
     # This node is used to lock a node of cypclass type around the evaluation of its subexpressions.
 
     # rlock_only    boolean
+    # needs_decref  boolean     used internally
 
     def __init__(self, arg, env=None, rlock_only=False):
         self.rlock_only = rlock_only
         self.type = arg.type
-        arg = arg.coerce_to_temp(env)
-        arg.postpone_subexpr_disposal = True
-        super(CoerceToLockedNode,self).__init__(arg)
+        temp_arg = arg.coerce_to_temp(env)
+        # Avoid incrementing the reference count when assigning to the temporary
+        # but ensure it will be decremented if it was already incremented previously.
+        self.needs_decref = not temp_arg.use_managed_ref
+        temp_arg.use_managed_ref = False
+        super(CoerceToLockedNode,self).__init__(temp_arg)
 
     def result(self):
         return self.arg.result()
 
+    def result_in_temp(self):
+        return self.arg.result_in_temp()
+
+    def coerce_to_temp(self):
+        return self
+
     def is_simple(self):
         return False
 
@@ -14180,34 +14203,25 @@ class CoerceToLockedNode(CoercionNode):
         return self.arg.may_be_none()
 
     def generate_result_code(self, code):
-        #XXX Code duplicated from Nodes.LockCypclassNode.
-        if self.arg.pos:
-            source_descr, lineno, colno = self.arg.pos
-            source_str = source_descr.get_description()
-            source_lines = source_descr.get_lines()
-            line_str = source_lines[lineno - 1]
-            col_str = "%s%s" % (' ' * colno, '^')
-            context = "%s:%d:%d\n%s%s" % (source_str, lineno, colno, line_str, col_str)
-            context = code.get_string_const(StringEncoding.EncodedString(context))
-        else:
-            context = "NULL"
-
-        # Create a scope to use scope bound resource management (RAII).
-        code.putln("{")
-
-        # Since each lock guard has its onw scope,
-        # a prefix is enough to prevent name collisions.
-        guard_code = "%sguard" % Naming.cypclass_lock_guard_prefix
+        # Acquire the lock.
         if self.rlock_only:
-            code.putln("Cy_rlock_guard %s(%s, %s);" % (guard_code, self.result(), context))
+            code.putln("%s->CyObject_RLOCK();" % self.result())
         else:
-            code.putln("Cy_wlock_guard %s(%s, %s);" % (guard_code, self.result(), context))
+            code.putln("%s->CyObject_WLOCK();" % self.result())
 
     def generate_disposal_code(self, code):
-        # Close the scope to release the lock.
-        code.putln("}")
-        # Dispose of subexpressions.
-        super(CoerceToLockedNode,self).generate_disposal_code(code)
+        # Release the lock.
+        if self.rlock_only:
+            code.putln("%s->CyObject_UNRLOCK();" % self.result())
+        else:
+            code.putln("%s->CyObject_UNWLOCK();" % self.result())
+
+        # The subexpressions of self.arg are disposed-of and freed
+        # as soon as self.arg is evaluated because it is a temporary.
+
+        # Decref only if previously incref-ed.
+        if self.needs_decref:
+            code.put_xdecref_clear(self.result(), self.ctype(), have_gil=not self.in_nogil_context)
 
 
 class ProxyNode(CoercionNode):

Cython/Compiler/Nodes.py

@@ -8597,6 +8597,9 @@ class LockCypclassNode(StatNode):
 
     child_attrs = ["body", "obj"]
 
+    nested = False
+    objs = None
+
     def analyse_declarations(self, env):
         self.body.analyse_declarations(env)
         self.obj.analyse_declarations(env)
@@ -8609,26 +8612,15 @@ class LockCypclassNode(StatNode):
     def generate_execution_code(self, code):
         self.obj.generate_evaluation_code(code)
 
-        if self.obj.pos:
-            source_descr, lineno, colno = self.obj.pos
-            source_str = source_descr.get_description()
-            source_lines = source_descr.get_lines()
-            line_str = source_lines[lineno - 1]
-            col_str = "%s%s" % (' ' * colno, '^')
-            context = "%s:%d:%d\n%s%s" % (source_str, lineno, colno, line_str, col_str)
-            context = code.get_string_const(EncodedString(context))
-        else:
-            context = "NULL"
-
         # Create a scope to use scope bound resource management (RAII).
         code.putln("{")
 
         # Each lock guard has its onw scope, so a prefix is enough to prevent name collisions
         guard_code = "%sguard" % Naming.cypclass_lock_guard_prefix
         if self.state == "rlocked":
-            code.putln("Cy_rlock_guard  %s(%s, %s);" % (guard_code, self.obj.result(), context))
+            code.putln("Cy_rlock_guard  %s(%s);" % (guard_code, self.obj.result()))
         elif self.state == "wlocked":
-            code.putln("Cy_wlock_guard  %s(%s ,%s);" % (guard_code, self.obj.result(), context))
+            code.putln("Cy_wlock_guard  %s(%s);" % (guard_code, self.obj.result()))
 
         self.body.generate_execution_code(code)
 

Cython/Compiler/Pipeline.py

@@ -212,9 +212,9 @@ def create_pipeline(context, mode, exclude_classes=()):
         _check_c_declarations,
         InlineDefNodeCalls(context),
         AnalyseExpressionsTransform(context),
-        CypclassLockTransform(context),
         FindInvalidUseOfFusedTypes(context),
         ExpandInplaceOperators(context),
+        CypclassLockTransform(context),
         IterationTransform(context),
         SwitchTransform(context),
         OptimizeBuiltinCalls(context),  ## Necessary?

Cython/Includes/libcythonplus/set.pxd

@@ -204,7 +204,7 @@ cdef cypclass cypset[V]:
 
     cypset[V] __ior__(self, const cypset[V] other) except ~:
         if self._active_iterators == 0:
-            self._elements.insert(other._elements.const_begin(), other._elements.end())
+            self._elements.insert(other._elements.const_begin(), other._elements.const_end())
             return self
         else:
             with gil:

tests/errors/cypclass_lock_error.pyx

@@ -9,35 +9,18 @@ cdef cypclass A checklock:
     void setter(self, int a):
         self.a = a
 
-cdef void take_write_locked(A obj):
-    pass
-
-cdef int take_read_locked(const A obj):
-    return 3
-
 def incorrect_locks():
     obj = A()
     obj.a = 3
     obj.getter()
     with rlocked obj:
         obj.setter(42)
-        take_write_locked(obj)
-    obj.a
-    take_read_locked(obj)
-
-
-cdef A global_cyobject
+    return obj.a
 
-cdef void global_lock_taking():
-    with wlocked global_cyobject:
-        global_cyobject.setter(global_cyobject.getter() + 1)
 
 _ERRORS = u"""
-20:4: Reference 'obj' is not correctly locked in this expression (write lock required)
-21:4: Reference 'obj' is not correctly locked in this expression (read lock required)
-23:8: Reference 'obj' is not correctly locked in this expression (write lock required)
-24:26: Reference 'obj' is not correctly locked in this expression (write lock required)
-25:4: Reference 'obj' is not correctly locked in this expression (read lock required)
-26:21: Reference 'obj' is not correctly locked in this expression (read lock required)
-32:17: Can only lock local variables or arguments
+8:15: Reference 'self' is not correctly locked in this expression (read lock required)
+10:8: Reference 'self' is not correctly locked in this expression (write lock required)
+14:4: Reference 'obj' is not correctly locked in this expression (write lock required)
+18:11: Reference 'obj' is not correctly locked in this expression (read lock required)
 """

tests/run/cypclass_acthon.pyx

-# mode: run
-# tag: cpp, cpp11, pthread
-# cython: experimental_cpp_class_def=True, language_level=2
-from libcpp.deque cimport deque
-
-ctypedef deque[ActhonMessageInterface] message_queue_t
-
-cdef extern from "<semaphore.h>" nogil:
-  ctypedef struct sem_t:
-    pass
-  int sem_init(sem_t *sem, int pshared, unsigned int value)
-  int sem_wait(sem_t *sem)
-  int sem_post(sem_t *sem)
-  int sem_destroy(sem_t* sem)
-
-
-cdef cypclass BasicQueue(ActhonQueueInterface) checklock:
-  message_queue_t* _queue
-
-  __init__(self):
-    self._queue = new message_queue_t()
-
-  __dealloc__(self):
-    del self._queue
-
-  bint is_empty(const self):
-    return self._queue.empty()
-
-  void push(self, ActhonMessageInterface message):
-    self._queue.push_back(message)
-    if message._sync_method is not NULL:
-      message._sync_method.insertActivity(message)
-
-  bint activate(self):
-    cdef bint one_message_processed
-    if self._queue.empty():
-      return False
-    # Note here that according to Cython refcount conventions,
-    # the front() method should have returned a new ref.
-    # This is obviously not the case, so if we do nothing
-    # we will, at the end of this function, loose a ref on the pointed object
-    # (as we will decref the thing pointed by next_message).
-    next_message = self._queue.front()
-    self._queue.pop_front()
-    one_message_processed = next_message.activate()
-    if one_message_processed:
-      if next_message._sync_method is not NULL:
-        next_sync_method = next_message._sync_method
-        with wlocked next_sync_method:
-          next_sync_method.removeActivity(next_message)
-    else:
-      self._queue.push_back(next_message)
-      # Don't forget to incref to avoid premature deallocation
-    return one_message_processed
-
-cdef cypclass NoneResult(ActhonResultInterface) checklock:
-  void pushVoidStarResult(self, void* result):
-    pass
-  void pushIntResult(self, int result):
-    pass
-  void* getVoidStarResult(const self):
-    return NULL
-  int getIntResult(const self):
-    return 0
-
-cdef cypclass WaitResult(ActhonResultInterface) checklock:
-  union result_t:
-    int int_val
-    void* ptr
-  result_t result
-  sem_t semaphore
-
-  __init__(self):
-    self.result.ptr = NULL
-    sem_init(&self.semaphore, 0, 0)
-
-  __dealloc__(self):
-    sem_destroy(&self.semaphore)
-
-  @staticmethod
-  ActhonResultInterface construct():
-    return WaitResult()
-
-  void pushVoidStarResult(self, void* result):
-    self.result.ptr = result
-    sem_post(&self.semaphore)
-
-  void pushIntResult(self, int result):
-    self.result.int_val = result
-    sem_post(&self.semaphore)
-
-  result_t _getRawResult(const self):
-    # We must ensure a result exists, but we can let others access it immediately
-    # The cast here is a way of const-casting (we're modifying the semaphore in a const method)
-    sem_wait(<sem_t*> &self.semaphore)
-    sem_post(<sem_t*> &self.semaphore)
-    return self.result
-
-  void* getVoidStarResult(const self):
-    res = self._getRawResult()
-    return res.ptr
-
-  int getIntResult(const self):
-    res = self._getRawResult()
-    return res.int_val
-
-cdef cypclass ActivityCounterSync(ActhonSyncInterface) checklock:
-  int count
-  ActivityCounterSync previous_sync
-
-  __init__(self, ActivityCounterSync prev = <ActivityCounterSync> NULL):
-    self.count = 0
-    self.previous_sync = prev
-
-  void insertActivity(self, ActhonMessageInterface msg):
-    self.count += 1
-
-  void removeActivity(self, ActhonMessageInterface msg):
-    self.count -= 1
-
-  bint isCompleted(const self):
-    return self.count == 0
-
-  bint isActivable(const self):
-    cdef bint res = True
-    if self.previous_sync is not NULL:
-      prev_sync = self.previous_sync
-      with rlocked prev_sync:
-        res = prev_sync.isCompleted()
-    return res
-
-cdef cypclass A checklock activable:
-    int a
-    __init__(self):
-        self.a = 0
-        self._active_result_class = WaitResult.construct
-        self._active_queue_class = BasicQueue()
-    int getter(const self):
-        return self.a
-    void setter(self, int a):
-        self.a = a
-
-def test_acthon_chain(n):
-    """
-    >>> test_acthon_chain(42)
-    42
-    """
-    cdef ActhonResultInterface res
-    cdef ActhonQueueInterface queue
-    sync1 = ActivityCounterSync()
-    with wlocked sync1:
-        after_sync1 = ActivityCounterSync(sync1)
-    obj = A()
-    with wlocked obj:
-        obj_actor = obj.__activate__()
-        with wlocked obj_actor, wlocked sync1, wlocked after_sync1:
-            # Pushing things in the queue
-            obj_actor.setter(sync1, n)
-            res = obj_actor.getter(after_sync1)
-    # Processing the queue
-    with rlocked obj:
-        queue = obj._active_queue_class
-    with wlocked queue:
-        while not queue.is_empty():
-            queue.activate()
-    print <int> res

tests/run/cypclass_lock.pyx

-# mode: run
-# tag: cpp, cpp11, pthread
-# cython: experimental_cpp_class_def=True, language_level=2
-
-cdef cypclass A checklock:
-    int a
-    __init__(self):
-        self.a = 0
-    int getter(const self):
-        return self.a
-    void setter(self, int a):
-        self.a = a
-
-def test_basic_locking():
-    """
-    >>> test_basic_locking()
-    0
-    """
-    obj = A()
-    with rlocked obj:
-        print obj.getter()
-
-cdef argument_recursivity(A obj, int arg):
-    if arg > 0:
-        obj.setter(obj.getter() + 1)
-        argument_recursivity(obj, arg - 1)
-
-def test_argument_recursivity(n):
-    """
-    >>> test_argument_recursivity(42)
-    42
-    """
-    obj = A()
-    with wlocked obj:
-        argument_recursivity(obj, n)
-        print obj.a
-
-cdef cypclass Container:
-    A object
-    __init__(self):
-        self.object = A()
-
-def test_lock_traversal(n):
-    """
-    >>> test_lock_traversal(42)
-    42
-    """
-    container = Container()
-    with rlocked container:
-        contained = container.object
-        with wlocked contained:
-            argument_recursivity(contained, n)
-            print contained.getter()