Bug 24798 – Under some circumstances, the compiler destroys the same object more than once

Status
NEW
Severity
regression
Priority
P1
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2024-10-06T10:51:46Z
Last change time
2024-12-13T19:37:54Z
Keywords
wrong-code
Assigned to
No Owner
Creator
Jonathan M Davis
Moved to GitHub: dmd#18273 →

Comments

Comment #0 by issues.dlang — 2024-10-06T10:51:46Z
This code fails the assertion in the destructor: --- void main() { import std.algorithm.comparison : equal; import std.algorithm.iteration : substitute; static struct S { int val; ~this() { assert(val != 99); val = 99; } } auto arr = [S(0), S(1), S(2), S(3)]; assert(equal(arr.substitute(S(1), S(42)), [S(0), S(42), S(2), S(3)])); } --- So, it would appear that substitute is somehow destroying an already destroyed value.
Comment #1 by issues.dlang — 2024-10-10T10:02:43Z
After dustmiting, I've reduced it to --- import std.range; void main() { static struct S { int val; ~this() { assert(val != 99, "Double Destroy"); val = 99; } } auto arr = [S(0), S(1), S(2), S(3)]; auto result = arr.mySubstitute(S(1), S(42)); auto front = result.front; } auto mySubstitute(R, Substs...)(R r, Substs) { struct MySubstituteElement { Substs substs; auto opCall(E)(E ) { return substs[0]; } } auto er = MySubstituteElement.init; return r.myMap!er; } template myMap(fun...) { auto myMap(Range)(Range r) { return MyMapResult!(fun, Range)(r); } } struct MyMapResult(alias fun, Range) { Range _input; @property empty() { return _input.empty; } void popFront() { } @property front() { return fun(_input); } } --- I could probably reduce it further if I understood the issue better, but any further transformations that I've tried have made the problem go away. In any case, given that all of the Phobos code was removed aside from import std.range to get the range primitives for arrays, it looks to me like this is a compiler bug. Also, trying it on run.dlang.org, it looks like it broke with dmd 2.068.2. Looking at the changelog - https://dlang.org/changelog/2.068.2.html - it looks like the problem was likely caused by reverting the fix for issue 14708: https://issues.dlang.org/show_bug.cgi?id=14708 I haven't dug into any of the details, so I really don't know what the exact issue is, but destructors need to not be running on an already destroyed object. It can potentially be worked around by explicitly setting a struct to its init value at the end of a destructor, but no one is going to think of doing that normally. I suspect that the main reason that this wasn't caught previously is simply because destructors aren't used all that frequently in D, but I ran into it at work when trying to add a destructor to an existing type, and this is one of the problems that I hit.
Comment #2 by kinke — 2024-10-10T11:33:46Z
I've reduced it to this: ``` struct S { int val; ~this() { import core.stdc.stdio : printf; printf("dtor %d, %p\n", val, &this); val = 99; } } void main() { auto r = makeR(); r.foo(); } auto makeR() { auto s = S(1); // allocated in GC closure return R!s(); // the frontend wrongly destructs `s` at the end of the function, see -vcg-ast } struct R(alias s) { void foo() { assert(s.val != 99, "already destroyed"); } } ``` So the problem is that in `makeR`, `s` is correctly allocated in a GC closure (because the return value has a hidden reference to it, so the local escapes its stack frame), but the frontend still eagerly destructs it before exiting the function. So calling `r.foo()` accesses a destructed `S` instance. AFAIK, variables in GC closures cannot be destructed, because there's no TypeInfo associated with the GC-allocated memory block. So the trade-off here would probably be that the escaping `s` local would be never destructed.
Comment #3 by kinke — 2024-10-10T11:47:43Z
To be clear, this is no double-destroy issue, as can be seen when printing the addresses of the destructed instances in the 2nd test case. The problem in the 2nd test case is that `MyMapResult.front()` returns a copy of `er.substs[0]`, with `er` already having been destructed (due to the actual issue), so it returns a copy of an `S(99)`, which then gets destructed, and the check then wrongly complaining about a double-destroy, while in reality it's a destruction of a copy of a destructed S instance.
Comment #4 by tim.dlang — 2024-10-10T13:05:54Z
This could be related to these issues: Issue 11382 - Bad closure variable with scoped destruction Issue 24120 - Closures break constructor/destructor safety DMD also contains a comment for destructors in closures: https://github.com/dlang/dmd/blob/00df883e9f32c34a070e4ff74783c33416848dc9/compiler/src/dmd/toir.d#L800 BUG: doesn't handle destructors for the local variables. ...
Comment #5 by robert.schadek — 2024-12-13T19:37:54Z
THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/18273 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB