From 907288d83d7e29e7def4de43fb9f32b39f971f66 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Sat, 24 Apr 2010 19:19:56 +0200 Subject: [PATCH] fix several ref-counting issues with closure variables, especially in error cases --- Cython/Compiler/Nodes.py | 53 +++++++++++++++++++--------- tests/run/closure_arg_type_error.pyx | 30 ++++++++++++++++ 2 files changed, 67 insertions(+), 16 deletions(-) create mode 100644 tests/run/closure_arg_type_error.pyx diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py index 653efc13..a0a8c508 100644 --- a/Cython/Compiler/Nodes.py +++ b/Cython/Compiler/Nodes.py @@ -1263,23 +1263,27 @@ class FuncDefNode(StatNode, BlockNode): env.use_utility_code(force_init_threads_utility_code) code.putln("PyGILState_STATE _save = PyGILState_Ensure();") # ----- Automatic lead-ins for certain special functions - if not lenv.nogil: - code.put_setup_refcount_context(self.entry.name) if profile: code.put_trace_call(self.entry.name, self.pos) if is_getbuffer_slot: self.getbuffer_init(code) - # ----- Create closure scope object + # ----- Create closure scope object, init ref-nanny if self.needs_closure: - code.putln("%s = (%s)%s->tp_new(%s, %s, NULL);" % ( - Naming.cur_scope_cname, - lenv.scope_class.type.declaration_code(''), - lenv.scope_class.type.typeptr_cname, - lenv.scope_class.type.typeptr_cname, - Naming.empty_tuple)) - # TODO: error handling + code.putln("%s = (%s)%s->tp_new(%s, %s, NULL); if (unlikely(!%s)) return %s;" % ( + Naming.cur_scope_cname, + lenv.scope_class.type.declaration_code(''), + lenv.scope_class.type.typeptr_cname, + lenv.scope_class.type.typeptr_cname, + Naming.empty_tuple, + Naming.cur_scope_cname, + # FIXME: what if the error return value is a Python value? + self.error_value())) + if not lenv.nogil: + code.put_setup_refcount_context(self.entry.name) code.put_gotref(Naming.cur_scope_cname) # Note that it is unsafe to decref the scope at this point. + elif not lenv.nogil: + code.put_setup_refcount_context(self.entry.name) if env.is_closure_scope: code.putln("%s = (%s)%s;" % ( outer_scope_cname, @@ -1294,8 +1298,9 @@ class FuncDefNode(StatNode, BlockNode): # If an argument is assigned to in the body, we must # incref it to properly keep track of refcounts. for entry in lenv.arg_entries: - if entry.type.is_pyobject and entry.assignments: - code.put_var_incref(entry) + if entry.type.is_pyobject: + if entry.assignments and not entry.in_closure: + code.put_var_incref(entry) # ----- Initialise local variables for entry in lenv.var_entries: if entry.type.is_pyobject and entry.init_to_none and entry.used: @@ -1403,8 +1408,6 @@ class FuncDefNode(StatNode, BlockNode): for entry in lenv.arg_entries: if entry.type.is_pyobject: if entry.in_closure: - if not entry.assignments: - code.put_var_incref(entry) code.put_var_giveref(entry) elif entry.assignments: code.put_var_decref(entry) @@ -2302,6 +2305,19 @@ class DefNode(FuncDefNode): else: code.put_var_decref(self.starstar_arg.entry) code.putln('__Pyx_AddTraceback("%s");' % self.entry.qualified_name) + # The arguments are put into the closure one after the + # other, so when type errors are found, all references in + # the closure instance must be properly ref-counted to + # facilitate generic closure instance deallocation. In + # the case of an argument type error, it's best to just + # Py_CLEAR() the already handled references, as this frees + # them as early as possible. + for arg in self.args: + if arg.type.is_pyobject and arg.entry.in_closure: + code.put_var_xdecref_clear(arg.entry) + if self.needs_closure: + code.put_decref(Naming.cur_scope_cname, self.local_scope.scope_class.type) + code.put_finish_refcount_context() code.putln("return %s;" % self.error_value()) if code.label_used(end_label): code.put_label(end_label) @@ -2310,7 +2326,10 @@ class DefNode(FuncDefNode): if arg.type.is_pyobject: if arg.is_generic: item = PyrexTypes.typecast(arg.type, PyrexTypes.py_object_type, item) - code.putln("%s = %s;" % (arg.entry.cname, item)) + entry = arg.entry + code.putln("%s = %s;" % (entry.cname, item)) + if entry.in_closure: + code.put_var_incref(entry) else: func = arg.type.from_py_function if func: @@ -2372,7 +2391,7 @@ class DefNode(FuncDefNode): self.star_arg.entry.cname)) if self.starstar_arg: code.putln("{") - code.put_decref(self.starstar_arg.entry.cname, py_object_type) + code.put_decref_clear(self.starstar_arg.entry.cname, py_object_type) code.putln("return %s;" % self.error_value()) code.putln("}") else: @@ -2716,6 +2735,8 @@ class DefNode(FuncDefNode): self.generate_arg_conversion(arg, code) elif arg.entry.in_closure: code.putln('%s = %s;' % (arg.entry.cname, arg.hdr_cname)) + if arg.type.is_pyobject: + code.put_var_incref(arg.entry) def generate_arg_conversion(self, arg, code): # Generate conversion code for one argument. diff --git a/tests/run/closure_arg_type_error.pyx b/tests/run/closure_arg_type_error.pyx new file mode 100644 index 00000000..a2a65594 --- /dev/null +++ b/tests/run/closure_arg_type_error.pyx @@ -0,0 +1,30 @@ + +# The arguments in f() are put into the closure one after the other, +# so the reference of 'o' is filled in before the type errors are +# found. This leaves a reference in the closure instance on error +# return, which must be properly ref-counted to facilitate generic +# closure deallocation. In the case of an argument type error, it's +# actually best to just Py_CLEAR() the already handled references, as +# this frees them as early as possible. + +# This test doesn't really check the ref-counting itself, it just +# reproduces the problem. + + +def func_with_typed_args(object o, int i, tuple t, double d): + """ + >>> g = func_with_typed_args(1, 2, (), 3.0) + >>> g() + (1, 2, (), 3.0) + + >>> g = func_with_typed_args(1, 'x', (), 3.0) + Traceback (most recent call last): + TypeError: an integer is required + + >>> g = func_with_typed_args(1, 2, 3, 3.0) + Traceback (most recent call last): + TypeError: Argument 't' has incorrect type (expected tuple, got int) + """ + def g(): + return o, i, t, d + return g -- 2.26.2