destructors not always properly run when a coroutine is suspended and then destroyed #63818
Comments
|
@llvm/issue-subscribers-c-20 |
|
@llvm/issue-subscribers-coroutines |
|
I am taking a look at this now. |
|
For those who are reading this, this issue has nothing to do with the less common comma operator syntax shown in the example. If you have a Also, yes, aggregate initialize is also list initialize. Confirmed to have the same bug. |
|
|
|
Interestingly, this is not the case with temporaries associated with function call parameters. This only happens with list/aggregate initialisation. coroutine foo() {
Printy a("a");
// bar(Printy("b"), co_await Awaiter{}); // Fine.
// (Printy("b"), co_await Awaiter{}); // Fine.
// Printies p1(Printy("b"), // Fine.
// co_await Awaiter{});
Printies p2{Printy("b"), // Leak.
co_await Awaiter{}};
co_return;
} |
|
I took a quick stab at this. The following diff resolves the example in the description that doesn't have a coroutine: diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index a5da0aa2965a..7c961ef91e6d 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -2423,7 +2423,7 @@ void CodeGenFunction::pushIrregularPartialArrayCleanup(llvm::Value *arrayBegin,
QualType elementType,
CharUnits elementAlign,
Destroyer *destroyer) {
- pushFullExprCleanup<IrregularPartialArrayDestroy>(EHCleanup,
+ pushFullExprCleanup<IrregularPartialArrayDestroy>(NormalAndEHCleanup,
arrayBegin, arrayEndPointer,
elementType, elementAlign,
destroyer);Output: But that diff causes the example with the coroutine to segfault (when compiled with |
|
Also, observe that gcc doesn't construct |
|
Yeah I noticed. In the following example, GCC creates Printies p2{Printy("b"), co_await Awaiter{"a1"}, Printy("c"), co_await Awaiter{"a2"}};Clang: GCC: |
|
I think now I understand the root cause and why simply changing the cleanup type would not solve the issue. struct Printies { const Printy &a; };
int main() {
Printies p1(Printy("a")); // dangling.
std::cout << "p1 ctor completed\n";
Printies p2{Printy("b")}; // lifetime extended to p2.
std::cout << "p2 ctor completed\n";
Printy arr[] = {Printy("c"), Printy("d")}; // lifetimes extended to arr.
std::cout << "arr[] ctor completed\n";
return 0;
}produces (clang is correct here): godbolt In the above example, the lifetime of temporary |
|
Thanks to the analysis of @usx95 There is another example not relevant with arrays: https://godbolt.org/z/4rnTj53qf. The key point here is that the construction is interrupted but not by exceptions. |
|
I do not understand the non-array example. It does not have temporaries with missed dtor. We do not have this problem with such control-flow with exceptions (https://godbolt.org/z/T9scoehev). |
Oh, yeah, you're correct. Sorry for misleading : ( |
|
@llvm/issue-subscribers-clang-codegen Author: Richard Smith (zygoloid)
[Example](https://godbolt.org/z/zezvTqY8v):
#include <coroutine>
#include <iostream>
struct coroutine {
struct promise_type;
std::coroutine_handle<promise_type> handle;
~coroutine() { handle.destroy(); }
};
struct coroutine::promise_type {
coroutine get_return_object() {
return {std::coroutine_handle<promise_type>::from_promise(*this)};
}
std::suspend_never initial_suspend() noexcept { return {}; }
std::suspend_never final_suspend() noexcept { return {}; }
void return_void() {}
void unhandled_exception() {}
};
struct Printy {
Printy(const char *name) : name(name) { std::cout << "Printy(" << name << ")\n"; }
Printy(const Printy&) = delete;
~Printy() { std::cout << "~Printy(" << name << ")\n"; }
const char *name;
};
int main() {
[] -> coroutine {
Printy a("a");
Printy arr[] = {
Printy("b"), Printy("c"),
(co_await std::suspend_always{}, Printy("d")),
Printy("e")
};
}();
}When the coroutine is destroyed after being suspended, This bug is not new with coroutines; the same thing happens with statement expressions: int main() {
Printy arr[] = { Printy("a"), ({ return 0; Printy("b"); }) };
}... never destroys |
|
Sent out partial resolution : #80698 List initialisation still has missing cleanups. Edit: Updated the patch to target all the missing cleanups. |
|
(previous change had to be reverted) |
|
Updated PR: #89154 |
) Latest diff: https://github.com/llvm/llvm-project/pull/89154/files/f1ab4c2677394bbfc985d9680d5eecd7b2e6a882..adf9bc902baddb156c83ce0f8ec03c142e806d45 We address two additional bugs here: ### Problem 1: Deactivated normal cleanup still runs, leading to double-free Consider the following: ```cpp struct A { }; struct B { B(const A&); }; struct S { A a; B b; }; int AcceptS(S s); void Accept2(int x, int y); void Test() { Accept2(AcceptS({.a = A{}, .b = A{}}), ({ return; 0; })); } ``` We add cleanups as follows: 1. push dtor for field `S::a` 2. push dtor for temp `A{}` (used by ` B(const A&)` in `.b = A{}`) 3. push dtor for field `S::b` 4. Deactivate 3 `S::b`-> This pops the cleanup. 5. Deactivate 1 `S::a` -> Does not pop the cleanup as *2* is top. Should create _active flag_!! 6. push dtor for `~S()`. 7. ... It is important to deactivate **5** using active flags. Without the active flags, the `return` will fallthrough it and would run both `~S()` and dtor `S::a` leading to **double free** of `~A()`. In this patch, we unconditionally emit active flags while deactivating normal cleanups. These flags are deleted later by the `AllocaTracker` if the cleanup is not emitted. ### Problem 2: Missing cleanup for conditional lifetime extension We push 2 cleanups for lifetime-extended cleanup. The first cleanup is useful if we exit from the middle of the expression (stmt-expr/coro suspensions). This is deactivated after full-expr, and a new cleanup is pushed, extending the lifetime of the temporaries (to the scope of the reference being initialized). If this lifetime extension happens to be conditional, then we use active flags to remember whether the branch was taken and if the object was initialized. Previously, we used a **single** active flag, which was used by both cleanups. This is wrong because the first cleanup will be forced to deactivate after the full-expr and therefore this **active** flag will always be **inactive**. The dtor for the lifetime extended entity would not run as it always sees an **inactive** flag. In this patch, we solve this using two separate active flags for both cleanups. Both of them are activated if the conditional branch is taken, but only one of them is deactivated after the full-expr. --- Fixes #63818 Fixes #88478 --- Previous PR logs: 1. #85398 2. #88670 3. #88751 4. #88884
|
Thanks for pointing out. This does seem like another blocker for stmt-expr but is not effecting C++20 coroutines. So let's keep this bug as closed and fixed. |
Example:
When the coroutine is destroyed after being suspended,
ais destroyed, butarr[0]andarr[1]are not. Clang does not in general properly create cleanups for non-exceptional control flow that occurs in the middle of an expression / an initializer. At least array initialization is missing cleanups here, but it'd be worth checking through all exception-only cleanups because most of them are probably incorrect. It looks like there are 15 places where we currently push an EH-only cleanup:This bug is not new with coroutines; the same thing happens with statement expressions:
... never destroys
arr[0]. But it seems more pressing now that it's reachable from standard C++20 code.The text was updated successfully, but these errors were encountered: