[mypyc] Add cleanup at end of init function #10590
Conversation
Add decrefs to the end of the generated init function to make sure we free everything we need to in the case of an error.
mypyc/codegen/emitmodule.py
Outdated
| for cl in module.classes: | ||
| if cl.is_generated: | ||
| type_struct = emitter.type_struct_name(cl) | ||
| type_structs.append(type_struct) |
There was a problem hiding this comment.
I think we need to collect the type structs even for the not generated ones. The is_generated classes (like those created for nested functions) get initialized here, but most classes get initialized by code we generated as IR, as part of the module initialization function.
(I can't remember why we don't arrange to do that for the generated ones; obviously I didn't feel great about that fact, which is why this block of code is labeled HACK :))
There was a problem hiding this comment.
I think this should be fixed by 24f5ce3.
mypyc/codegen/emitmodule.py
Outdated
| @@ -918,7 +920,13 @@ def generate_module_def(self, emitter: Emitter, module_name: str, module: Module | |||
|
|
|||
| emitter.emit_line('return {};'.format(module_static)) | |||
| emitter.emit_lines('fail:', | |||
| '{} = NULL;'.format(module_static), | |||
| 'Py_XDECREF({});'.format(module_static), | |||
There was a problem hiding this comment.
Do Py_CLEAR for these, which will do an Py_XDECREF and NULL out the variable. (By virtue of being a macro that does not behave like a function...)
| 'Py_XDECREF(modname);') | ||
| # the type objects returned from CPyType_FromTemplate are all new references | ||
| # so we have to decref them | ||
| for t in type_structs: |
There was a problem hiding this comment.
We should also do the finals, too. Those need to be reset to whatever the proper error value for the type is.
There was a problem hiding this comment.
Could you take a look at ea8b1b2? I added clean up for some finals but I think there are some types that we should clean up which that doesn't cover.
There was a problem hiding this comment.
I think the right thing to do is to call out to emit_dec_ref, which knows how to decref everything: https://github.com/python/mypy/blob/master/mypyc/codegen/emit.py#L344
And then assign the result of c_undefined_value to the variable, to clear it out
There was a problem hiding this comment.
Yeah, that does look a lot cleaner. This should be fixed in ebe20ed.
modname might be uninitialized if PyModule_Create fails with an error, so initialize modname to NULL at the beginning of the function so that Py_XDECREF does nothing.
Free the type structs associated with all classes in the case of an error, regardless of whether those classes are generated.
Use Py_CLEAR instead of using Py_XDECREF on the module object we created and then setting it to NULL because Py_CLEAR does the same thing.
Make sure we clean up finals if we run into an error while running the initialization code.
Use Py_CLEAR on the module name and all of the type structs to make sure that those are set to NULL if we run into an error.
mypyc/codegen/emitmodule.py
Outdated
| 'return NULL;') | ||
| 'Py_CLEAR({});'.format(module_static), | ||
| 'Py_CLEAR(modname);') | ||
| # the type objects returned from CPyType_FromTemplate are all new references |
There was a problem hiding this comment.
I think this comment has drifted away from the code it is describing
There was a problem hiding this comment.
This should be fixed in ff8d287.
The comment about why we have to decref and clear type structs got moved in one of the previous commits, so move it back to the right place.
Improve the handling of final values for types other than python objects and tagged integers by using emit_dec_ref and c_undefined_value. In particular, this should handle tuples correctly and make sure to clear tagged integers correctly.
This frees everything created in the generated initialization function so that we don't leak memory in the case of an error during initialization, as mentioned in #10586 (review).