[clang] 54225c4 - [Coroutines] Fix premature conversion of return object
Bruno Cardoso Lopes via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 9 14:19:03 PST 2023
Author: Bruno Cardoso Lopes
Date: 2023-03-09T14:18:26-08:00
New Revision: 54225c457a336b1609c6d064b2b606a9238a28b9
URL: https://github.com/llvm/llvm-project/commit/54225c457a336b1609c6d064b2b606a9238a28b9
DIFF: https://github.com/llvm/llvm-project/commit/54225c457a336b1609c6d064b2b606a9238a28b9.diff
LOG: [Coroutines] Fix premature conversion of return object
Fix https://github.com/llvm/llvm-project/issues/56532
Effectively, this reverts behavior introduced in https://reviews.llvm.org/D117087,
which did two things:
1. Change delayed to early conversion of return object.
2. Introduced RVO possibilities because of early conversion.
This patches fixes (1) and removes (2). I already worked on a follow up for (2)
in a separated patch. I believe it's important to split these two because if the RVO
causes any problems we can explore reverting (2) while maintaining (1).
Notes on some testcase changes:
- `pr59221.cpp` changed to `-O1` so we can check that the front-end honors
the value checked for. Sounds like `-O3` without RVO is more likely
to work with LLVM optimizations...
- Comment out delete members `coroutine-no-move-ctor.cpp` since behavior
now requires copies again.
Differential Revision: https://reviews.llvm.org/D145639
Added:
Modified:
clang/include/clang/AST/StmtCXX.h
clang/lib/AST/StmtCXX.cpp
clang/lib/CodeGen/CGCoroutine.cpp
clang/lib/Sema/SemaCoroutine.cpp
clang/lib/Sema/TreeTransform.h
clang/test/CodeGenCoroutines/coro-gro.cpp
clang/test/CodeGenCoroutines/pr59221.cpp
clang/test/SemaCXX/coroutine-no-move-ctor.cpp
clang/test/SemaCXX/coroutines.cpp
clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/AST/StmtCXX.h b/clang/include/clang/AST/StmtCXX.h
index 2c71f86768963..05dfac2b50c3f 100644
--- a/clang/include/clang/AST/StmtCXX.h
+++ b/clang/include/clang/AST/StmtCXX.h
@@ -326,6 +326,7 @@ class CoroutineBodyStmt final
OnFallthrough, ///< Handler for control flow falling off the body.
Allocate, ///< Coroutine frame memory allocation.
Deallocate, ///< Coroutine frame memory deallocation.
+ ResultDecl, ///< Declaration holding the result of get_return_object.
ReturnValue, ///< Return value for thunk function: p.get_return_object().
ReturnStmt, ///< Return statement for the thunk function.
ReturnStmtOnAllocFailure, ///< Return statement if allocation failed.
@@ -352,6 +353,7 @@ class CoroutineBodyStmt final
Stmt *OnFallthrough = nullptr;
Expr *Allocate = nullptr;
Expr *Deallocate = nullptr;
+ Stmt *ResultDecl = nullptr;
Expr *ReturnValue = nullptr;
Stmt *ReturnStmt = nullptr;
Stmt *ReturnStmtOnAllocFailure = nullptr;
@@ -404,6 +406,7 @@ class CoroutineBodyStmt final
Expr *getDeallocate() const {
return cast_or_null<Expr>(getStoredStmts()[SubStmt::Deallocate]);
}
+ Stmt *getResultDecl() const { return getStoredStmts()[SubStmt::ResultDecl]; }
Expr *getReturnValueInit() const {
return cast<Expr>(getStoredStmts()[SubStmt::ReturnValue]);
}
diff --git a/clang/lib/AST/StmtCXX.cpp b/clang/lib/AST/StmtCXX.cpp
index 33b0421ad1016..a3ae5392f54bc 100644
--- a/clang/lib/AST/StmtCXX.cpp
+++ b/clang/lib/AST/StmtCXX.cpp
@@ -117,6 +117,7 @@ CoroutineBodyStmt::CoroutineBodyStmt(CoroutineBodyStmt::CtorArgs const &Args)
SubStmts[CoroutineBodyStmt::OnFallthrough] = Args.OnFallthrough;
SubStmts[CoroutineBodyStmt::Allocate] = Args.Allocate;
SubStmts[CoroutineBodyStmt::Deallocate] = Args.Deallocate;
+ SubStmts[CoroutineBodyStmt::ResultDecl] = Args.ResultDecl;
SubStmts[CoroutineBodyStmt::ReturnValue] = Args.ReturnValue;
SubStmts[CoroutineBodyStmt::ReturnStmt] = Args.ReturnStmt;
SubStmts[CoroutineBodyStmt::ReturnStmtOnAllocFailure] =
diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp
index 9b233c1807cf1..38167cc74a7f3 100644
--- a/clang/lib/CodeGen/CGCoroutine.cpp
+++ b/clang/lib/CodeGen/CGCoroutine.cpp
@@ -467,6 +467,71 @@ struct CallCoroDelete final : public EHScopeStack::Cleanup {
};
}
+namespace {
+struct GetReturnObjectManager {
+ CodeGenFunction &CGF;
+ CGBuilderTy &Builder;
+ const CoroutineBodyStmt &S;
+
+ Address GroActiveFlag;
+ CodeGenFunction::AutoVarEmission GroEmission;
+
+ GetReturnObjectManager(CodeGenFunction &CGF, const CoroutineBodyStmt &S)
+ : CGF(CGF), Builder(CGF.Builder), S(S), GroActiveFlag(Address::invalid()),
+ GroEmission(CodeGenFunction::AutoVarEmission::invalid()) {}
+
+ // The gro variable has to outlive coroutine frame and coroutine promise, but,
+ // it can only be initialized after coroutine promise was created, thus, we
+ // split its emission in two parts. EmitGroAlloca emits an alloca and sets up
+ // cleanups. Later when coroutine promise is available we initialize the gro
+ // and sets the flag that the cleanup is now active.
+ void EmitGroAlloca() {
+ auto *GroDeclStmt = dyn_cast<DeclStmt>(S.getResultDecl());
+ if (!GroDeclStmt) {
+ // If get_return_object returns void, no need to do an alloca.
+ return;
+ }
+
+ auto *GroVarDecl = cast<VarDecl>(GroDeclStmt->getSingleDecl());
+
+ // Set GRO flag that it is not initialized yet
+ GroActiveFlag = CGF.CreateTempAlloca(Builder.getInt1Ty(), CharUnits::One(),
+ "gro.active");
+ Builder.CreateStore(Builder.getFalse(), GroActiveFlag);
+
+ GroEmission = CGF.EmitAutoVarAlloca(*GroVarDecl);
+
+ // Remember the top of EHStack before emitting the cleanup.
+ auto old_top = CGF.EHStack.stable_begin();
+ CGF.EmitAutoVarCleanups(GroEmission);
+ auto top = CGF.EHStack.stable_begin();
+
+ // Make the cleanup conditional on gro.active
+ for (auto b = CGF.EHStack.find(top), e = CGF.EHStack.find(old_top); b != e;
+ b++) {
+ if (auto *Cleanup = dyn_cast<EHCleanupScope>(&*b)) {
+ assert(!Cleanup->hasActiveFlag() && "cleanup already has active flag?");
+ Cleanup->setActiveFlag(GroActiveFlag);
+ Cleanup->setTestFlagInEHCleanup();
+ Cleanup->setTestFlagInNormalCleanup();
+ }
+ }
+ }
+
+ void EmitGroInit() {
+ if (!GroActiveFlag.isValid()) {
+ // No Gro variable was allocated. Simply emit the call to
+ // get_return_object.
+ CGF.EmitStmt(S.getResultDecl());
+ return;
+ }
+
+ CGF.EmitAutoVarInit(GroEmission);
+ Builder.CreateStore(Builder.getTrue(), GroActiveFlag);
+ }
+};
+} // namespace
+
static void emitBodyAndFallthrough(CodeGenFunction &CGF,
const CoroutineBodyStmt &S, Stmt *Body) {
CGF.EmitStmt(Body);
@@ -533,6 +598,13 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
CGM.getIntrinsic(llvm::Intrinsic::coro_begin), {CoroId, Phi});
CurCoro.Data->CoroBegin = CoroBegin;
+ // We need to emit `get_return_object` first. According to:
+ // [dcl.fct.def.coroutine]p7
+ // The call to get_return_object is sequenced before the call to
+ // initial_suspend and is invoked at most once.
+ GetReturnObjectManager GroManager(*this, S);
+ GroManager.EmitGroAlloca();
+
CurCoro.Data->CleanupJD = getJumpDestInCurrentScope(RetBB);
{
CGDebugInfo *DI = getDebugInfo();
@@ -570,23 +642,8 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
// promise local variable was not emitted yet.
CoroId->setArgOperand(1, PromiseAddrVoidPtr);
- // ReturnValue should be valid as long as the coroutine's return type
- // is not void. The assertion could help us to reduce the check later.
- assert(ReturnValue.isValid() == (bool)S.getReturnStmt());
- // Now we have the promise, initialize the GRO.
- // We need to emit `get_return_object` first. According to:
- // [dcl.fct.def.coroutine]p7
- // The call to get_return_object is sequenced before the call to
- // initial_suspend and is invoked at most once.
- //
- // So we couldn't emit return value when we emit return statment,
- // otherwise the call to get_return_object wouldn't be in front
- // of initial_suspend.
- if (ReturnValue.isValid()) {
- EmitAnyExprToMem(S.getReturnValue(), ReturnValue,
- S.getReturnValue()->getType().getQualifiers(),
- /*IsInit*/ true);
- }
+ // Now we have the promise, initialize the GRO
+ GroManager.EmitGroInit();
EHStack.pushCleanup<CallCoroEnd>(EHCleanup);
@@ -649,12 +706,8 @@ void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
llvm::Function *CoroEnd = CGM.getIntrinsic(llvm::Intrinsic::coro_end);
Builder.CreateCall(CoroEnd, {NullPtr, Builder.getFalse()});
- if (Stmt *Ret = S.getReturnStmt()) {
- // Since we already emitted the return value above, so we shouldn't
- // emit it again here.
- cast<ReturnStmt>(Ret)->setRetValue(nullptr);
+ if (Stmt *Ret = S.getReturnStmt())
EmitStmt(Ret);
- }
// LLVM require the frontend to mark the coroutine.
CurFn->setPresplitCoroutine();
diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index 0dcfbd5281d1d..3df2af25f6f36 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -1736,6 +1736,7 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() {
if (Res.isInvalid())
return false;
+ this->ResultDecl = Res.get();
return true;
}
@@ -1748,12 +1749,55 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() {
return false;
}
- StmtResult ReturnStmt = S.BuildReturnStmt(Loc, ReturnValue);
+ // StmtResult ReturnStmt = S.BuildReturnStmt(Loc, ReturnValue);
+ auto *GroDecl = VarDecl::Create(
+ S.Context, &FD, FD.getLocation(), FD.getLocation(),
+ &S.PP.getIdentifierTable().get("__coro_gro"), GroType,
+ S.Context.getTrivialTypeSourceInfo(GroType, Loc), SC_None);
+ GroDecl->setImplicit();
+
+ S.CheckVariableDeclarationType(GroDecl);
+ if (GroDecl->isInvalidDecl())
+ return false;
+
+ InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl);
+ ExprResult Res =
+ S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
+ if (Res.isInvalid())
+ return false;
+
+ Res = S.ActOnFinishFullExpr(Res.get(), /*DiscardedValue*/ false);
+ if (Res.isInvalid())
+ return false;
+
+ S.AddInitializerToDecl(GroDecl, Res.get(),
+ /*DirectInit=*/false);
+
+ S.FinalizeDeclaration(GroDecl);
+
+ // Form a declaration statement for the return declaration, so that AST
+ // visitors can more easily find it.
+ StmtResult GroDeclStmt =
+ S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(GroDecl), Loc, Loc);
+ if (GroDeclStmt.isInvalid())
+ return false;
+
+ this->ResultDecl = GroDeclStmt.get();
+
+ ExprResult declRef = S.BuildDeclRefExpr(GroDecl, GroType, VK_LValue, Loc);
+ if (declRef.isInvalid())
+ return false;
+
+ StmtResult ReturnStmt = S.BuildReturnStmt(Loc, declRef.get());
+
if (ReturnStmt.isInvalid()) {
noteMemberDeclaredHere(S, ReturnValue, Fn);
return false;
}
+ if (cast<clang::ReturnStmt>(ReturnStmt.get())->getNRVOCandidate() == GroDecl)
+ GroDecl->setNRVOVariable(true);
+
this->ReturnStmt = ReturnStmt.get();
return true;
}
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index c6a3af1b72b07..590f0b4d2474c 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -8051,6 +8051,12 @@ TreeTransform<Derived>::TransformCoroutineBodyStmt(CoroutineBodyStmt *S) {
return StmtError();
Builder.Deallocate = DeallocRes.get();
+ assert(S->getResultDecl() && "ResultDecl must already be built");
+ StmtResult ResultDecl = getDerived().TransformStmt(S->getResultDecl());
+ if (ResultDecl.isInvalid())
+ return StmtError();
+ Builder.ResultDecl = ResultDecl.get();
+
if (auto *ReturnStmt = S->getReturnStmt()) {
StmtResult Res = getDerived().TransformStmt(ReturnStmt);
if (Res.isInvalid())
diff --git a/clang/test/CodeGenCoroutines/coro-gro.cpp b/clang/test/CodeGenCoroutines/coro-gro.cpp
index fad75c9433076..ddcf112f0da6b 100644
--- a/clang/test/CodeGenCoroutines/coro-gro.cpp
+++ b/clang/test/CodeGenCoroutines/coro-gro.cpp
@@ -46,13 +46,14 @@ void doSomething() noexcept;
// CHECK: define{{.*}} i32 @_Z1fv(
int f() {
// CHECK: %[[RetVal:.+]] = alloca i32
+ // CHECK: %[[GroActive:.+]] = alloca i1
// CHECK: %[[Size:.+]] = call i64 @llvm.coro.size.i64()
// CHECK: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]])
+ // CHECK: store i1 false, ptr %[[GroActive]]
// CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_typeC1Ev(
- // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(ptr sret(%struct.GroType) align {{[0-9]+}} %[[GRO:.+]],
- // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv({{.*}}[[GRO]]
- // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+ // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(
+ // CHECK: store i1 true, ptr %[[GroActive]]
Cleanup cleanup;
doSomething();
@@ -68,7 +69,18 @@ int f() {
// CHECK: %[[Mem:.+]] = call ptr @llvm.coro.free(
// CHECK: call void @_ZdlPv(ptr noundef %[[Mem]])
- // CHECK: coro.ret:
+ // Initialize retval from Gro and destroy Gro
+
+ // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv(
+ // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+ // CHECK: %[[IsActive:.+]] = load i1, ptr %[[GroActive]]
+ // CHECK: br i1 %[[IsActive]], label %[[CleanupGro:.+]], label %[[Done:.+]]
+
+ // CHECK: [[CleanupGro]]:
+ // CHECK: call void @_ZN7GroTypeD1Ev(
+ // CHECK: br label %[[Done]]
+
+ // CHECK: [[Done]]:
// CHECK: %[[LoadRet:.+]] = load i32, ptr %[[RetVal]]
// CHECK: ret i32 %[[LoadRet]]
}
diff --git a/clang/test/CodeGenCoroutines/pr59221.cpp b/clang/test/CodeGenCoroutines/pr59221.cpp
index ae5f6fdbdea92..e0e3de559a403 100644
--- a/clang/test/CodeGenCoroutines/pr59221.cpp
+++ b/clang/test/CodeGenCoroutines/pr59221.cpp
@@ -2,7 +2,7 @@
//
// REQUIRES: x86-registered-target
//
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O3 -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O1 -S -emit-llvm -o - | FileCheck %s
#include "Inputs/coroutine.h"
diff --git a/clang/test/SemaCXX/coroutine-no-move-ctor.cpp b/clang/test/SemaCXX/coroutine-no-move-ctor.cpp
index 824dea375ebde..08933f4df7a8e 100644
--- a/clang/test/SemaCXX/coroutine-no-move-ctor.cpp
+++ b/clang/test/SemaCXX/coroutine-no-move-ctor.cpp
@@ -15,10 +15,13 @@ class invoker {
};
using promise_type = invoker_promise;
invoker() {}
- invoker(const invoker &) = delete;
- invoker &operator=(const invoker &) = delete;
- invoker(invoker &&) = delete;
- invoker &operator=(invoker &&) = delete;
+ // TODO: implement RVO for get_return_object type matching
+ // function return type.
+ //
+ // invoker(const invoker &) = delete;
+ // invoker &operator=(const invoker &) = delete;
+ // invoker(invoker &&) = delete;
+ // invoker &operator=(invoker &&) = delete;
};
invoker f() {
diff --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp
index e480c0d34593a..782f4b2f63333 100644
--- a/clang/test/SemaCXX/coroutines.cpp
+++ b/clang/test/SemaCXX/coroutines.cpp
@@ -934,7 +934,7 @@ struct std::coroutine_traits<int, mismatch_gro_type_tag2> {
extern "C" int f(mismatch_gro_type_tag2) {
// cxx2b-error at -1 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
- // cxx14_20-error at -2 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
+ // cxx14_20-error at -2 {{cannot initialize return object of type 'int' with an lvalue of type 'void *'}}
co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
}
diff --git a/clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp b/clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
index 4d52bdca7ca93..e96aae4fefc6b 100644
--- a/clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
+++ b/clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
@@ -13,8 +13,6 @@ struct task {
explicit task(promise_type& p) { throw 1; p.return_val = this; }
- task(const task&) = delete;
-
T value;
};
More information about the cfe-commits
mailing list