llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-coroutines Author: etiennep-chromium <details> <summary>Changes</summary> On Win32 x86, arguments passed by value with non-trivial constructors/destructors require `inalloca`. If a coroutine suspends during the evaluation of these arguments, the stack frame is popped, destroying the already constructed arguments. This fix detects when a call inside a coroutine requires `inalloca` and contains suspend points in its arguments. In this case, we: 1. Force all arguments of the call to be evaluated into temporaries in the coroutine frame (which survive suspension). 2. Delay the `llvm.stacksave` to the end of argument evaluation (after resume). 3. Allocate the `inalloca` struct on the stack after resume and copy the temporaries into it. 4. Emit lifetime markers (`llvm.lifetime.start`/`llvm.lifetime.end`) for the `inalloca` alloca to prevent it from being moved to the coroutine frame by CoroSplit. 5. Safely clean up the temporaries in the frame. If an argument is non-copyable/non-movable, we now emit a compilation error. BUG=LLVM#<!-- -->59382 --- Full diff: https://github.com/llvm/llvm-project/pull/212331.diff 9 Files Affected: - (modified) clang/include/clang/AST/Expr.h (+4) - (modified) clang/lib/AST/Expr.cpp (+16) - (modified) clang/lib/CodeGen/CGCall.cpp (+87-4) - (modified) clang/lib/CodeGen/CGExpr.cpp (+5) - (modified) clang/lib/CodeGen/CGExprAgg.cpp (+10) - (modified) clang/lib/CodeGen/CGExprCXX.cpp (+10-1) - (modified) clang/lib/CodeGen/CodeGenFunction.h (+12) - (modified) clang/lib/Sema/SemaExpr.cpp (+40) - (added) clang/test/CodeGenCoroutines/coro-win32-inalloca.cpp (+93) ``````````diff diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h index f95f87cc4e8e0..9a6347d08f790 100644 --- a/clang/include/clang/AST/Expr.h +++ b/clang/include/clang/AST/Expr.h @@ -247,6 +247,10 @@ class Expr : public ValueStmt { return static_cast<bool>(getDependence() & ExprDependence::Error); } + /// Whether this expression contains a coroutine suspend point + /// (co_await or co_yield). + bool containsCoroutineSuspendPoints() const; + /// getExprLoc - Return the preferred location for the arrow when diagnosing /// a problem with a generic expression. SourceLocation getExprLoc() const LLVM_READONLY; diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index 9a9a76e265f6a..22a697fe09327 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -3998,6 +3998,22 @@ bool Expr::HasSideEffects(const ASTContext &Ctx, return false; } +static bool containsCoroutineSuspendPoints(const Stmt *S) { + if (!S) + return false; + if (isa<CoawaitExpr>(S) || isa<CoyieldExpr>(S) || + isa<DependentCoawaitExpr>(S)) + return true; + for (const Stmt *Child : S->children()) + if (Child && containsCoroutineSuspendPoints(Child)) + return true; + return false; +} + +bool Expr::containsCoroutineSuspendPoints() const { + return ::containsCoroutineSuspendPoints(this); +} + FPOptions Expr::getFPFeaturesInEffect(const LangOptions &LO) const { if (auto Call = dyn_cast<CallExpr>(this)) return Call->getFPFeaturesInEffect(LO); diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp index a29f7aab0e58b..17f9c8abd7497 100644 --- a/clang/lib/CodeGen/CGCall.cpp +++ b/clang/lib/CodeGen/CGCall.cpp @@ -28,7 +28,9 @@ #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/ExprCXX.h" #include "clang/AST/RecordLayout.h" +#include "clang/AST/RecursiveASTVisitor.h" #include "clang/Basic/CodeGenOptions.h" #include "clang/Basic/TargetInfo.h" #include "clang/CodeGen/CGFunctionInfo.h" @@ -5029,6 +5031,46 @@ static bool isObjCMethodWithTypeParams(const ObjCMethodDecl *method) { } #endif +static const MaterializeTemporaryExpr *getMTEToPreEvaluate(const Expr *E) { + while (true) { + E = E->IgnoreParens(); + if (auto *Cleanups = dyn_cast<ExprWithCleanups>(E)) { + E = Cleanups->getSubExpr(); + continue; + } + if (auto *Bind = dyn_cast<CXXBindTemporaryExpr>(E)) { + E = Bind->getSubExpr(); + continue; + } + break; + } + + if (auto *CE = dyn_cast<CXXConstructExpr>(E)) { + if (CE->getNumArgs() > 0) { + const Expr *Arg = CE->getArg(0); + while (true) { + Arg = Arg->IgnoreParens(); + if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Arg)) + return MTE; + if (auto *Cleanups = dyn_cast<ExprWithCleanups>(Arg)) { + Arg = Cleanups->getSubExpr(); + continue; + } + if (auto *Bind = dyn_cast<CXXBindTemporaryExpr>(Arg)) { + Arg = Bind->getSubExpr(); + continue; + } + if (auto *ICE = dyn_cast<ImplicitCastExpr>(Arg)) { + Arg = ICE->getSubExpr(); + continue; + } + break; + } + } + } + return nullptr; +} + /// EmitCallArgs - Emit call arguments for a function. void CodeGenFunction::EmitCallArgs( CallArgList &Args, PrototypeWrapper Prototype, @@ -5127,7 +5169,30 @@ void CodeGenFunction::EmitCallArgs( std::swap(Args.back(), *(&Args.back() - 1)); }; - // Insert a stack save if we're going to need any inalloca args. + SmallVector<const MaterializeTemporaryExpr *, 4> MTEsToPreEvaluate; + if (isCoroutine() && hasInAllocaArgs(CGM, ExplicitCC, ArgTypes)) { + for (const Expr *A : ArgRange) { + if (auto *MTE = getMTEToPreEvaluate(A)) { + MTEsToPreEvaluate.push_back(MTE); + } + } + } + + if (!MTEsToPreEvaluate.empty()) { + if (LeftToRight) { + for (const MaterializeTemporaryExpr *MTE : MTEsToPreEvaluate) { + LValue LV = EmitMaterializeTemporaryExpr(MTE); + PreEvaluatedMaterializedTemporaries[MTE] = LV; + } + } else { + for (int i = MTEsToPreEvaluate.size() - 1; i >= 0; --i) { + const MaterializeTemporaryExpr *MTE = MTEsToPreEvaluate[i]; + LValue LV = EmitMaterializeTemporaryExpr(MTE); + PreEvaluatedMaterializedTemporaries[MTE] = LV; + } + } + } + if (hasInAllocaArgs(CGM, ExplicitCC, ArgTypes)) { assert(getTarget().getTriple().getArch() == llvm::Triple::x86 && "inalloca only supported on x86"); @@ -5175,6 +5240,10 @@ void CodeGenFunction::EmitCallArgs( // Reverse the writebacks to match the MSVC ABI. Args.reverseWritebacks(); } + + for (const auto *MTE : MTEsToPreEvaluate) { + PreEvaluatedMaterializedTemporaries.erase(MTE); + } } namespace { @@ -5673,17 +5742,28 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, llvm::Instruction *IP = CallArgs.getStackBase(); llvm::AllocaInst *AI; if (IP) { - IP = IP->getNextNode(); - AI = new llvm::AllocaInst(ArgStruct, DL.getAllocaAddrSpace(), "argmem", - IP->getIterator()); + if (llvm::Instruction *Next = IP->getNextNode()) { + AI = new llvm::AllocaInst(ArgStruct, DL.getAllocaAddrSpace(), "argmem", + Next->getIterator()); + } else { + AI = new llvm::AllocaInst(ArgStruct, DL.getAllocaAddrSpace(), "argmem", + IP->getParent()); + } } else { AI = CreateTempAlloca(ArgStruct, "argmem"); } auto Align = CallInfo.getArgStructAlignment(); AI->setAlignment(Align.getAsAlign()); AI->setUsedWithInAlloca(true); + if (isCoroutine()) { + AI->setMetadata(llvm::LLVMContext::MD_coro_outside_frame, + llvm::MDNode::get(CGM.getLLVMContext(), {})); + } assert(AI->isUsedWithInAlloca() && !AI->isStaticAlloca()); ArgMemory = RawAddress(AI, ArgStruct, Align); + if (isCoroutine()) { + EmitLifetimeStart(AI); + } } ClangToLLVMArgMapping IRFunctionArgs(CGM.getContext(), CallInfo); @@ -6567,6 +6647,9 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, // The stack cleanup for inalloca arguments has to run out of the normal // lexical order, so deactivate it and run it manually here. CallArgs.freeArgumentMemory(*this); + if (isCoroutine() && ArgMemory.isValid()) { + EmitLifetimeEnd(ArgMemory.getPointer()); + } // Extract the return value. RValue Ret; diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index 9201e40bc13a1..2d05121c24f4f 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -521,6 +521,11 @@ static bool isAAPCS(const TargetInfo &TargetInfo) { LValue CodeGenFunction:: EmitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *M) { + auto It = PreEvaluatedMaterializedTemporaries.find(M); + if (It != PreEvaluatedMaterializedTemporaries.end()) { + return It->second; + } + const Expr *E = M->getSubExpr(); assert((!M->getExtendingDecl() || !isa<VarDecl>(M->getExtendingDecl()) || diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp index bc35ffdaad2bd..0c0dde5aa5178 100644 --- a/clang/lib/CodeGen/CGExprAgg.cpp +++ b/clang/lib/CodeGen/CGExprAgg.cpp @@ -777,6 +777,16 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType, void AggExprEmitter::VisitMaterializeTemporaryExpr( MaterializeTemporaryExpr *E) { + if (CGF.hasPreEvaluatedTemporary(E)) { + LValue SrcLV = CGF.getPreEvaluatedTemporary(E); + QualType Type = E->getType(); + assert((!Type->getAsCXXRecordDecl() || + Type->getAsCXXRecordDecl()->isTriviallyCopyable()) && + "Non-trivially copyable MTE should not be visited as aggregate when " + "pre-evaluated"); + EmitFinalDestCopy(Type, SrcLV); + return; + } Visit(E->getSubExpr()); } diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp index e400a5c5a49c5..7410e2bc18f51 100644 --- a/clang/lib/CodeGen/CGExprCXX.cpp +++ b/clang/lib/CodeGen/CGExprCXX.cpp @@ -637,7 +637,16 @@ void CodeGenFunction::EmitCXXConstructExpr(const CXXConstructExpr *E, return; // Elide the constructor if we're constructing from a temporary. - if (getLangOpts().ElideConstructors && E->isElidable()) { + bool DisableElision = false; + if (E->getNumArgs() > 0) { + if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>( + E->getArg(0)->IgnoreImpCasts())) { + if (hasPreEvaluatedTemporary(MTE)) + DisableElision = true; + } + } + + if (getLangOpts().ElideConstructors && E->isElidable() && !DisableElision) { // FIXME: This only handles the simplest case, where the source object // is passed directly as the first argument to the constructor. // This should also handle stepping though implicit casts and diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h index 3fc9052adb87b..4517b03be8bee 100644 --- a/clang/lib/CodeGen/CodeGenFunction.h +++ b/clang/lib/CodeGen/CodeGenFunction.h @@ -1774,6 +1774,8 @@ class CodeGenFunction : public CodeGenTypeCache { /// expressions. llvm::DenseMap<const OpaqueValueExpr *, LValue> OpaqueLValues; llvm::DenseMap<const OpaqueValueExpr *, RValue> OpaqueRValues; + llvm::DenseMap<const MaterializeTemporaryExpr *, LValue> + PreEvaluatedMaterializedTemporaries; // VLASizeMap - This keeps track of the associated size for each VLA type. // We track this by the size expression rather than the type itself because @@ -3106,6 +3108,16 @@ class CodeGenFunction : public CodeGenTypeCache { /// already been emitted. bool isOpaqueValueEmitted(const OpaqueValueExpr *E); + bool hasPreEvaluatedTemporary(const MaterializeTemporaryExpr *M) const { + return PreEvaluatedMaterializedTemporaries.count(M); + } + LValue getPreEvaluatedTemporary(const MaterializeTemporaryExpr *M) { + auto It = PreEvaluatedMaterializedTemporaries.find(M); + assert(It != PreEvaluatedMaterializedTemporaries.end() && + "MTE is not pre-evaluated"); + return It->second; + } + /// Get the index of the current ArrayInitLoopExpr, if any. llvm::Value *getArrayInitIndex() { return ArrayInitIndex; } diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index ed3d27b5adc27..5f71404fb8c5f 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -6250,6 +6250,30 @@ Sema::ConvertArgumentsForCall(CallExpr *Call, Expr *Fn, return false; } +static bool isWin32InAllocaRecord(ASTContext &Context, QualType Ty) { + const RecordType *RT = Ty->getAs<RecordType>(); + if (!RT) + return false; + const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(RT->getDecl()); + if (!RD) + return false; + + const llvm::Triple &Triple = Context.getTargetInfo().getTriple(); + if (Triple.getArch() != llvm::Triple::x86 || !Triple.isOSWindows()) + return false; + if (!Context.getTargetInfo().getCXXABI().isMicrosoft()) + return false; + + if (RD->canPassInRegisters()) + return false; + + TypeInfo Info = Context.getTypeInfo(Context.getCanonicalTagType(RD)); + if (Info.isAlignRequired() && Info.Align > 4) + return false; // passed indirectly + + return true; +} + bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, FunctionDecl *FDecl, const FunctionProtoType *Proto, unsigned FirstParam, ArrayRef<Expr *> Args, @@ -6257,6 +6281,15 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, FunctionDecl *FDecl, VariadicCallType CallType, bool AllowExplicit, bool IsListInitialization) { unsigned NumParams = Proto->getNumParams(); + bool CallHasSuspend = false; + if (getCurFunction() && getCurFunction()->isCoroutine()) { + for (const Expr *A : Args) { + if (A && A->containsCoroutineSuspendPoints()) { + CallHasSuspend = true; + break; + } + } + } bool Invalid = false; size_t ArgIx = 0; // Continue to check argument types (even if we have too few/many args). @@ -6316,6 +6349,13 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, FunctionDecl *FDecl, : diag::warn_obt_discarded_at_function_boundary) << Arg->getType() << ProtoArgType; } + if (CallHasSuspend && Arg->isPRValue() && + isWin32InAllocaRecord(Context, ProtoArgType)) { + ExprResult Materialized = + CreateMaterializeTemporaryExpr(Arg->getType(), Arg, false); + if (!Materialized.isInvalid()) + Arg = Materialized.get(); + } ExprResult ArgE = PerformCopyInitialization( Entity, SourceLocation(), Arg, IsListInitialization, AllowExplicit); diff --git a/clang/test/CodeGenCoroutines/coro-win32-inalloca.cpp b/clang/test/CodeGenCoroutines/coro-win32-inalloca.cpp new file mode 100644 index 0000000000000..34d1d0d81d943 --- /dev/null +++ b/clang/test/CodeGenCoroutines/coro-win32-inalloca.cpp @@ -0,0 +1,93 @@ +// RUN: %clang_cc1 -std=c++20 -triple=i686-pc-windows-msvc -emit-llvm -o - %s -disable-llvm-passes | FileCheck %s + +namespace std { +template <typename R, typename... Args> +struct coroutine_traits { + using promise_type = typename R::promise_type; +}; + +template <class Promise = void> struct coroutine_handle { + coroutine_handle() = default; + static coroutine_handle from_address(void *) noexcept; +}; +template <> struct coroutine_handle<void> { + static coroutine_handle from_address(void *) noexcept; + coroutine_handle() = default; + template <class PromiseType> + coroutine_handle(coroutine_handle<PromiseType>) noexcept; +}; +} // namespace std + +struct suspend_always { + bool await_ready() noexcept { return false; } + void await_suspend(std::coroutine_handle<>) noexcept {} + void await_resume() noexcept {} +}; + +struct suspend_never { + bool await_ready() noexcept { return true; } + void await_suspend(std::coroutine_handle<>) noexcept {} + void await_resume() noexcept {} +}; + +struct task { + struct promise_type { + task get_return_object() { return {}; } + suspend_never initial_suspend() { return {}; } + suspend_never final_suspend() noexcept { return {}; } + void return_void() {} + void unhandled_exception() {} + }; +}; + +struct Noisy { + int val; + Noisy(int v); + Noisy(const Noisy&) = delete; + Noisy(Noisy&& o) noexcept; + ~Noisy(); +}; + +struct Awaiter { + bool await_ready() noexcept { return false; } + void await_suspend(std::coroutine_handle<>) noexcept {} + Noisy await_resume() noexcept; +}; + +void consume_two(Noisy x, Noisy y); + +// CHECK-LABEL: define dso_local void @"?my_task@@YA?AUtask@@XZ"( +task my_task() { + // CHECK: %[[MTE_Y:.+]] = alloca %struct.Noisy, + // CHECK: %[[MTE_X:.+]] = alloca %struct.Noisy, + + // Evaluate Noisy(42) before suspend: + // CHECK: call x86_thiscallcc noundef ptr @"??0Noisy@@QAE@H@Z"(ptr {{[^,]*}} %[[MTE_Y]], i32 noundef 42) + + // Suspend for co_await Awaiter{}: + // CHECK: call void @llvm.coro.await.suspend.void( + // CHECK: call i8 @llvm.coro.suspend( + + // After resume: + // CHECK: call x86_thiscallcc void @"?await_resume@Awaiter@@QAE?AUNoisy@@XZ"(ptr {{[^,]*}} %{{.*}}, ptr dead_on_unwind writable sret(%struct.Noisy) align 4 %[[MTE_X]]) + + // Allocate inalloca: + // CHECK: %[[STACKSAVE:.+]] = call ptr @llvm.stacksave.p0() + // CHECK: %[[ARGMEM:.+]] = alloca inalloca <{ %struct.Noisy, %struct.Noisy }>, align 4, !coro.outside.frame ![[METADATA_NUM:[0-9]+]] + + // Move y (pre-evaluated Noisy(42)) to inalloca: + // CHECK: %[[GEP_Y:.+]] = getelementptr inbounds nuw <{ %struct.Noisy, %struct.Noisy }>, ptr %[[ARGMEM]], i32 0, i32 1 + // CHECK: call x86_thiscallcc noundef ptr @"??0Noisy@@QAE@$$QAU0@@Z"(ptr {{[^,]*}} %[[GEP_Y]], ptr noundef nonnull align 4 dereferenceable(4) %[[MTE_Y]]) + + // Move x (co_await Awaiter{} result) to inalloca: + // CHECK: %[[GEP_X:.+]] = getelementptr inbounds nuw <{ %struct.Noisy, %struct.Noisy }>, ptr %[[ARGMEM]], i32 0, i32 0 + // CHECK: call x86_thiscallcc noundef ptr @"??0Noisy@@QAE@$$QAU0@@Z"(ptr {{[^,]*}} %[[GEP_X]], ptr noundef nonnull align 4 dereferenceable(4) %[[MTE_X]]) + + // Lifetime start and call: + // CHECK: call void @llvm.lifetime.start.p0(ptr %[[ARGMEM]]) + // CHECK: call void @"?consume_two@@YAXUNoisy@@0@Z"(ptr inalloca(<{ %struct.Noisy, %struct.Noisy }>) %[[ARGMEM]]) + + consume_two(co_await Awaiter{}, Noisy(42)); +} + +// CHECK: ![[METADATA_NUM]] = !{} `````````` </details> https://github.com/llvm/llvm-project/pull/212331 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
