Skip to content

[mypyc] Fix object finalization #19749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 2, 2025
Merged

[mypyc] Fix object finalization #19749

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef00dde
[mypyc] Fix object finalization
cdce8p Aug 27, 2025
5b92c8b
Move PyErr_WriteUnraisable call
cdce8p Aug 28, 2025
d6b6d79
Code review
cdce8p Aug 28, 2025
9a2bb0a
Update test output
cdce8p Aug 28, 2025
f62a220
Fix segfault
cdce8p Sep 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 11 additions & 18 deletions mypyc/codegen/emitclass.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,7 +844,17 @@ def generate_dealloc_for_class(
emitter.emit_line("{")
if has_tp_finalize:
emitter.emit_line("if (!PyObject_GC_IsFinalized((PyObject *)self)) {")
emitter.emit_line("Py_TYPE(self)->tp_finalize((PyObject *)self);")
emitter.emit_line("PyObject *type, *value, *traceback;")
emitter.emit_line("PyErr_Fetch(&type, &value, &traceback);")
emitter.emit_line("PyObject_CallFinalizerFromDealloc((PyObject *)self);")
# CPython interpreter uses PyErr_WriteUnraisable: https://docs.python.org/3/c-api/exceptions.html#c.PyErr_WriteUnraisable
# However, the message is slightly different due to the way mypyc compiles classes.
# CPython interpreter prints: Exception ignored in: <function F.__del__ at 0x100aed940>
# mypyc prints: Exception ignored in: <slot wrapper '__del__' of 'F' objects>
emitter.emit_line("if (PyErr_Occurred() != NULL) {")
emitter.emit_line("PyErr_WriteUnraisable((PyObject *)self);")
emitter.emit_line("}")
emitter.emit_line("PyErr_Restore(type, value, traceback);")
Copy link
Member

@ZeroIntensity ZeroIntensity Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this code to the done label below.

Copy link
Collaborator Author

@cdce8p cdce8p Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work unfortunately. If I move it, other tests start to fail. E.g. mypyc/test/test_run.py::TestRun::run-exceptions.test::testTryExcept with:

  Traceback (most recent call last): (diff)
    File "testutil.py", line 46, in assertRaises (diff)
      yield (diff)
    File "driver.py", line 42, in <module> (diff)
      list(iter_exception()) (diff)
      ~~~~^^^^^^^^^^^^^^^^^^ (diff)
  SystemError: error return without exception set (diff)
   (diff)
  During handling of the above exception, another exception occurred: (diff)
   (diff)
  Traceback (most recent call last): (diff)
    File "driver.py", line 41, in <module> (diff)
      with assertRaises(IndexError): (diff)
           ~~~~~~~~~~~~^^^^^^^^^^^^ (diff)
    File "/Users/marc/Develop/01_temp/cpython/Lib/contextlib.py", line 162, in __exit__ (diff)
      self.gen.throw(value) (diff)
      ~~~~~~~~~~~~~~^^^^^^^ (diff)
    File "testutil.py", line 48, in assertRaises (diff)
      assert type(e) is typ, f"{e!r} is not a {typ.__name__}" (diff)
             ^^^^^^^^^^^^^^ (diff)
  AssertionError: SystemError('error return without exception set') is not a IndexError (diff)

So I think PyErr_Restore needs to happen earlier.

Copy link
Member

@ZeroIntensity ZeroIntensity Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that's not making sense to me. Is the done label not getting hit in all cases?

Copy link
Collaborator Author

@cdce8p cdce8p Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, but some cleanup functions from mypyc seem to require that the exception is set properly.
Just the goto done; call is never used AFAICT.

Copy link
Member

@ZeroIntensity ZeroIntensity Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's a little messy. As long as they don't overwrite the exception, I guess it's fine.

emitter.emit_line("}")
emitter.emit_line("PyObject_GC_UnTrack(self);")
if cl.reuse_freed_instance:
Expand Down Expand Up @@ -884,30 +894,13 @@ def generate_finalize_for_class(
emitter.emit_line("static void")
emitter.emit_line(f"{finalize_func_name}(PyObject *self)")
emitter.emit_line("{")
emitter.emit_line("PyObject *type, *value, *traceback;")
emitter.emit_line("PyErr_Fetch(&type, &value, &traceback);")
emitter.emit_line(
"{}{}{}(self);".format(
emitter.get_group_prefix(del_method.decl),
NATIVE_PREFIX,
del_method.cname(emitter.names),
)
)
emitter.emit_line("if (PyErr_Occurred() != NULL) {")
emitter.emit_line('PyObject *del_str = PyUnicode_FromString("__del__");')
emitter.emit_line(
"PyObject *del_method = (del_str == NULL) ? NULL : _PyType_Lookup(Py_TYPE(self), del_str);"
)
# CPython interpreter uses PyErr_WriteUnraisable: https://docs.python.org/3/c-api/exceptions.html#c.PyErr_WriteUnraisable
# However, the message is slightly different due to the way mypyc compiles classes.
# CPython interpreter prints: Exception ignored in: <function F.__del__ at 0x100aed940>
# mypyc prints: Exception ignored in: <slot wrapper '__del__' of 'F' objects>
emitter.emit_line("PyErr_WriteUnraisable(del_method);")
emitter.emit_line("Py_XDECREF(del_method);")
emitter.emit_line("Py_XDECREF(del_str);")
emitter.emit_line("}")
# PyErr_Restore also clears exception raised in __del__.
emitter.emit_line("PyErr_Restore(type, value, traceback);")
emitter.emit_line("}")


Expand Down
Loading