# HG changeset patch # User scoder <stefan_ml@behnel.de> # Date 1593621880 -7200 # Wed Jul 01 18:44:40 2020 +0200 # Node ID 381a79dcc7c34d92ad65b0b63cb6eb4381d3eb40 # Parent 3e2edaf86c8b62de8ef5e516fce441829aedce65 Validate and fix temp releasing (GH-3708) (GH-3717) * Validate and fix temp releasing (GH-3708) Backports 92147baf11071352ffbfa475d0d21e091753e628. * Fix a temp leak in the type init code. * Fix temp leaks in fused types initialisation code. * Correctly release the buffer index temps allocated for index calculations. * Make tests fails hard if a temp variable is not released at the end of a generated function. * Fix temp leak in switch statement code. * Make end-to-end tests fail on refnanny output. * Fix result temp leak in PyTypeTestNode. * Fix result temp leak in external type/function import code and enable the refnanny check for them. * Fix temp leak when try-return-finally is used in generators. * Make it explicit when an allocated temp is not meant to be reused. * Fix temp leak when assigning to the real/imag attributes of complex numbers. * Fix temp leak when assigning to a memoryview slice. * Clean up "num_threads" result temp in parallel section, not only in prange loop. * Fix temp leak in Pythran buffer setitem code. * Simplify NumPyMethodCallNode since it does not need the Python function anymore. Previously, it generated code that needlessly looked up the Python function without actually using it. * Fix temp leak when deleting C++ objects. * Add a test that actually reusing temps when deleting C++ objects works correctly. diff --git a/Cython/Compiler/Code.py b/Cython/Compiler/Code.py --- a/Cython/Compiler/Code.py +++ b/Cython/Compiler/Code.py @@ -718,9 +718,10 @@ self.can_trace = False self.gil_owned = True - self.temps_allocated = [] # of (name, type, manage_ref, static) - self.temps_free = {} # (type, manage_ref) -> list of free vars with same type/managed status - self.temps_used_type = {} # name -> (type, manage_ref) + self.temps_allocated = [] # of (name, type, manage_ref, static) + self.temps_free = {} # (type, manage_ref) -> list of free vars with same type/managed status + self.temps_used_type = {} # name -> (type, manage_ref) + self.zombie_temps = set() # temps that must not be reused after release self.temp_counter = 0 self.closure_temps = None @@ -735,6 +736,20 @@ self.should_declare_error_indicator = False self.uses_error_indicator = False + # safety checks + + def validate_exit(self): + # validate that all allocated temps have been freed + if self.temps_allocated: + leftovers = self.temps_in_use() + if leftovers: + msg = "TEMPGUARD: Temps left over at end of '%s': %s" % (self.scope.name, ', '.join([ + '%s [%s]' % (name, ctype) + for name, ctype, is_pytemp in sorted(leftovers)]), + ) + #print(msg) + raise RuntimeError(msg) + # labels def new_label(self, name=None): @@ -804,7 +819,7 @@ # temp handling - def allocate_temp(self, type, manage_ref, static=False): + def allocate_temp(self, type, manage_ref, static=False, reusable=True): """ Allocates a temporary (which may create a new one or get a previously allocated and released one of the same type). Type is simply registered @@ -823,6 +838,8 @@ This is only used when allocating backing store for a module-level C array literals. + if reusable=False, the temp will not be reused after release. + A C string referring to the variable is returned. """ if type.is_const and not type.is_reference: @@ -838,7 +855,7 @@ manage_ref = False freelist = self.temps_free.get((type, manage_ref)) - if freelist is not None and freelist[0]: + if reusable and freelist is not None and freelist[0]: result = freelist[0].pop() freelist[1].remove(result) else: @@ -847,9 +864,11 @@ result = "%s%d" % (Naming.codewriter_temp_prefix, self.temp_counter) if result not in self.names_taken: break self.temps_allocated.append((result, type, manage_ref, static)) + if not reusable: + self.zombie_temps.add(result) self.temps_used_type[result] = (type, manage_ref) if DebugFlags.debug_temp_code_comments: - self.owner.putln("/* %s allocated (%s) */" % (result, type)) + self.owner.putln("/* %s allocated (%s)%s */" % (result, type, "" if reusable else " - zombie")) if self.collect_temps_stack: self.collect_temps_stack[-1].add((result, type)) @@ -868,10 +887,12 @@ self.temps_free[(type, manage_ref)] = freelist if name in freelist[1]: raise RuntimeError("Temp %s freed twice!" % name) - freelist[0].append(name) + if name not in self.zombie_temps: + freelist[0].append(name) freelist[1].add(name) if DebugFlags.debug_temp_code_comments: - self.owner.putln("/* %s released */" % name) + self.owner.putln("/* %s released %s*/" % ( + name, " - zombie" if name in self.zombie_temps else "")) def temps_in_use(self): """Return a list of (cname,type,manage_ref) tuples of temp names and their type diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py --- a/Cython/Compiler/ExprNodes.py +++ b/Cython/Compiler/ExprNodes.py @@ -4334,6 +4334,7 @@ pythran_indexing_code(self.indices), op, rhs.pythran_result())) + code.funcstate.release_temp(obj) return # Used from generate_assignment_code and InPlaceAssignmentNode @@ -4374,11 +4375,11 @@ code.putln("%s = (PyObject *) *%s;" % (self.result(), self.buffer_ptr_code)) code.putln("__Pyx_INCREF((PyObject*)%s);" % self.result()) - def free_temps(self, code): + def free_subexpr_temps(self, code): for temp in self.index_temps: code.funcstate.release_temp(temp) self.index_temps = () - super(BufferIndexNode, self).free_temps(code) + super(BufferIndexNode, self).free_subexpr_temps(code) class MemoryViewIndexNode(BufferIndexNode): @@ -4655,6 +4656,7 @@ self.dst.generate_evaluation_code(code) self._generate_assignment_code(rhs, code) self.dst.generate_disposal_code(code) + self.dst.free_temps(code) rhs.generate_disposal_code(code) rhs.free_temps(code) @@ -5558,7 +5560,7 @@ env.add_include_file(pythran_get_func_include_file(function)) return NumPyMethodCallNode.from_node( self, - function=function, + function_cname=pythran_functor(function), arg_tuple=self.arg_tuple, type=PythranExpr(pythran_func_type(function, self.arg_tuple.args)), ) @@ -5963,13 +5965,13 @@ code.funcstate.release_temp(self.opt_arg_struct) -class NumPyMethodCallNode(SimpleCallNode): +class NumPyMethodCallNode(ExprNode): # Pythran call to a NumPy function or method. # - # function ExprNode the function/method to call - # arg_tuple TupleNode the arguments as an args tuple - - subexprs = ['function', 'arg_tuple'] + # function_cname string the function/method to call + # arg_tuple TupleNode the arguments as an args tuple + + subexprs = ['arg_tuple'] is_temp = True may_return_none = True @@ -5977,7 +5979,6 @@ code.mark_pos(self.pos) self.allocate_temp_result(code) - self.function.generate_evaluation_code(code) assert self.arg_tuple.mult_factor is None args = self.arg_tuple.args for arg in args: @@ -5988,7 +5989,7 @@ code.putln("new (&%s) decltype(%s){%s{}(%s)};" % ( self.result(), self.result(), - pythran_functor(self.function), + self.function_cname, ", ".join(a.pythran_result() for a in args))) @@ -7277,6 +7278,8 @@ self.member.upper(), self.obj.result_as(self.obj.type), rhs.result_as(self.ctype()))) + rhs.generate_disposal_code(code) + rhs.free_temps(code) else: select_code = self.result() if self.type.is_pyobject and self.use_managed_ref: @@ -8127,21 +8130,19 @@ return t def allocate_temp_result(self, code): - if self.type.is_array and self.in_module_scope: - self.temp_code = code.funcstate.allocate_temp( - self.type, manage_ref=False, static=True) + if self.type.is_array: + if self.in_module_scope: + self.temp_code = code.funcstate.allocate_temp( + self.type, manage_ref=False, static=True, reusable=False) + else: + # To be valid C++, we must allocate the memory on the stack + # manually and be sure not to reuse it for something else. + # Yes, this means that we leak a temp array variable. + self.temp_code = code.funcstate.allocate_temp( + self.type, manage_ref=False, reusable=False) else: SequenceNode.allocate_temp_result(self, code) - def release_temp_result(self, env): - if self.type.is_array: - # To be valid C++, we must allocate the memory on the stack - # manually and be sure not to reuse it for something else. - # Yes, this means that we leak a temp array variable. - pass - else: - SequenceNode.release_temp_result(self, env) - def calculate_constant_result(self): if self.mult_factor: raise ValueError() # may exceed the compile time memory @@ -13076,9 +13077,18 @@ def generate_post_assignment_code(self, code): self.arg.generate_post_assignment_code(code) + def allocate_temp_result(self, code): + pass + + def release_temp_result(self, code): + pass + def free_temps(self, code): self.arg.free_temps(code) + def free_subexpr_temps(self, code): + self.arg.free_subexpr_temps(code) + class NoneCheckNode(CoercionNode): # This node is used to check that a Python object is not None and diff --git a/Cython/Compiler/FusedNode.py b/Cython/Compiler/FusedNode.py --- a/Cython/Compiler/FusedNode.py +++ b/Cython/Compiler/FusedNode.py @@ -878,17 +878,23 @@ (self.resulting_fused_function.result(), self.__signatures__.result())) code.put_giveref(self.__signatures__.result()) + self.__signatures__.generate_post_assignment_code(code) + self.__signatures__.free_temps(code) self.fused_func_assignment.generate_execution_code(code) # Dispose of results self.resulting_fused_function.generate_disposal_code(code) + self.resulting_fused_function.free_temps(code) self.defaults_tuple.generate_disposal_code(code) + self.defaults_tuple.free_temps(code) self.code_object.generate_disposal_code(code) + self.code_object.free_temps(code) for default in self.defaults: if default is not None: default.generate_disposal_code(code) + default.free_temps(code) def annotate(self, code): for stat in self.stats: diff --git a/Cython/Compiler/ModuleNode.py b/Cython/Compiler/ModuleNode.py --- a/Cython/Compiler/ModuleNode.py +++ b/Cython/Compiler/ModuleNode.py @@ -2949,6 +2949,7 @@ module.qualified_name, temp, code.error_goto(self.pos))) + code.put_gotref(temp) for entry in entries: if env is module: cname = entry.cname @@ -2959,7 +2960,8 @@ 'if (__Pyx_ImportVoidPtr(%s, "%s", (void **)&%s, "%s") < 0) %s' % ( temp, entry.name, cname, signature, code.error_goto(self.pos))) - code.putln("Py_DECREF(%s); %s = 0;" % (temp, temp)) + code.put_decref_clear(temp, py_object_type) + code.funcstate.release_temp(temp) def generate_c_function_import_code_for_module(self, module, env, code): # Generate import code for all exported C functions in a cimported module. @@ -2977,6 +2979,7 @@ module.qualified_name, temp, code.error_goto(self.pos))) + code.put_gotref(temp) for entry in entries: code.putln( 'if (__Pyx_ImportFunction(%s, "%s", (void (**)(void))&%s, "%s") < 0) %s' % ( @@ -2985,7 +2988,8 @@ entry.cname, entry.type.signature_string(), code.error_goto(self.pos))) - code.putln("Py_DECREF(%s); %s = 0;" % (temp, temp)) + code.put_decref_clear(temp, py_object_type) + code.funcstate.release_temp(temp) def generate_type_init_code(self, env, code): # Generate type import code for extern extension types diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py --- a/Cython/Compiler/Nodes.py +++ b/Cython/Compiler/Nodes.py @@ -5866,6 +5866,7 @@ arg.generate_evaluation_code(code) code.putln("delete %s;" % arg.result()) arg.generate_disposal_code(code) + arg.free_temps(code) # else error reported earlier def annotate(self, code): @@ -6410,6 +6411,8 @@ # generate the switch statement, so shouldn't be bothered). code.putln("default: break;") code.putln("}") + self.test.generate_disposal_code(code) + self.test.free_temps(code) def generate_function_definitions(self, env, code): self.test.generate_function_definitions(env, code) @@ -7673,9 +7676,10 @@ if self.func_return_type.is_pyobject: code.putln("%s = 0;" % ret_temp) code.funcstate.release_temp(ret_temp) - ret_temp = None if self.in_generator: self.put_error_uncatcher(code, exc_vars) + for cname in exc_vars: + code.funcstate.release_temp(cname) if not self.finally_clause.is_terminator: code.put_goto(old_label) @@ -8772,6 +8776,11 @@ self.begin_of_parallel_control_block_point = None self.begin_of_parallel_control_block_point_after_decls = None + if self.num_threads is not None: + # FIXME: is it the right place? should not normally produce code. + self.num_threads.generate_disposal_code(code) + self.num_threads.free_temps(code) + # Firstly, always prefer errors over returning, continue or break if self.error_label_used: c.putln("const char *%s = NULL; int %s = 0, %s = 0;" % self.parallel_pos_info) @@ -9151,7 +9160,7 @@ # And finally, release our privates and write back any closure # variables - for temp in start_stop_step + (self.chunksize, self.num_threads): + for temp in start_stop_step + (self.chunksize,): if temp is not None: temp.generate_disposal_code(code) temp.free_temps(code) diff --git a/tests/run/cpp_classes.pyx b/tests/run/cpp_classes.pyx --- a/tests/run/cpp_classes.pyx +++ b/tests/run/cpp_classes.pyx @@ -212,6 +212,7 @@ cdef int f(int x): return x + def test_nested_del(): """ >>> test_nested_del() @@ -220,3 +221,15 @@ v.push_back(new vector[int]()) del v[0] del create_to_delete()[f(f(0))] + + +def test_nested_del_repeat(): + """ + >>> test_nested_del_repeat() + """ + cdef vector[vector_int_ptr] v + v.push_back(new vector[int]()) + del v[0] + del create_to_delete()[f(f(0))] + del create_to_delete()[f(f(0))] + del create_to_delete()[f(f(0))]