Repository navigation
Fix #23979: GC never finalizes a class whose only destructors are field destructors - #23980
Conversation
…e field destructors `_d_newclassT` marked a new class instance as finalizable only when the class had a user-declared destructor (`__dtor`). A class whose destruction comes solely from its fields only gets a compiler-generated `__xdtor`, so it was allocated without `BlkAttr.FINALIZE` and the GC never ran its fields' destructors, although `destroy` and `scope` worked. This regressed in 2.103.0 when `_d_newclass` became a template (dlang#14837); the old runtime function checked `ClassFlags.hasDtor`, which includes field destructors. Also check for `__xdtor`.
rainers
left a comment
rainers
left a comment
There was a problem hiding this comment.
LGTM, but I'm not sure whether the failing tests are spurious or whether this uncovers an issue with the runtime shared across DLLs or with the tests.
I've restarted the failing builds.
|
#22399 appears to be biting again? Now |
|
This looks like a real bug and not a flaky test. Both Windows jobs crash in druntime/test/shared/src/dllgc.d with an access violation, in debug and release: My guess is that the test's I tried to reproduce the Windows crash under Wine, but Wine does something different and the test passes there. I don't have a Windows setup, so if anyone can confirm on real Windows, I'd appreciate it. Otherwise I can push a change that keeps the task in a |
|
Would it be ok for me to push a untested fix and have the CI run it without having to setup windows locally for testing? |
…druntime rt_termSharedModule did not run the finalizers whose code is in the module being unloaded, so the GC later called into the unmapped DLL and crashed. Run them after the module destructors, as the ELF implementation does. This affected any finalizable object whose class is defined in such a DLL; druntime/test/shared/src/dllgc.d started hitting it once classes with only field destructors became finalizable.
|
Of course. |
|
I could not reproduce it locally on Windows, but I also think the Task is missing a reference to keep it alive. So saving it into a global variable might help. |
4 participants
Fixes #23979.
_d_newclassTonly setBlkAttr.FINALIZEwhen the class had a user-declared destructor (__dtor). A class whose destruction comes only from its fields has just the compiler-generated__xdtor, so the GC never finalized it. This also checks for__xdtor, which matches what the old_d_newclassdid by checkingClassFlags.hasDtor.The new unittest sits next to the existing
_d_newclassTtests and checks the block'sFINALIZEattribute for a field-only class, a class with~this(), and a plain class. It fails on master without the fix, and druntime's unittests pass with it.Two things worth knowing when reviewing:
-preview=dtorfieldsbecame the default #18037, which already affects classes with~this()on DMD and all classes on LDC; this change brings field-only classes in line with those rather than introducing anything new.hasMember(T, "__xdtor")can be true for a class whose only "destructible" field is a zero-length array. The worst case is a finalizer that has nothing to do (at least I think so).