Index: lib/CodeGen/CGCleanup.cpp =================================================================== --- lib/CodeGen/CGCleanup.cpp +++ lib/CodeGen/CGCleanup.cpp @@ -417,8 +417,41 @@ EHStack.popNullFixups(); } +/// Ensure that *VP dominates the fallthrough insert point after cleanups have +/// run. Abnormal exits (break, return, etc) from GNU statement expressions +/// create the possibility that the insert point during expression evaluation +/// may not dominate the fallthrough insertion point after running cleanups. +/// Callers that wish to use values live across cleanups can use this method to +/// defend against this without creating temporaries in the common case. +void CodeGenFunction::RunCleanupsScope::ensureDominatingValue(llvm::Value **VP) { + // Nothing to do if no cleanups were actually required. + if (!requiresCleanups()) + return; + + // No need to ensure void expressions or constants. + auto *Inst = dyn_cast_or_null(*VP); + if (!Inst) + return; + + // Check if any of the cleanup scopes that we're going to run have branches. + // If they do, we need to reload this value. + bool HasBranches = false; + for (auto I = CGF.EHStack.begin(), E = CGF.EHStack.find(CleanupStackDepth); + I != E; ++I) { + HasBranches = cast(*I).hasBranches(); + if (HasBranches) + break; + } + if (!HasBranches) + return; + + ValuesToReload.push_back(VP); +} + /// Pops cleanup blocks until the given savepoint is reached. -void CodeGenFunction::PopCleanupBlocks(EHScopeStack::stable_iterator Old) { +void CodeGenFunction::PopCleanupBlocks( + EHScopeStack::stable_iterator Old, + ArrayRef ValuesToReload) { assert(Old.isValid()); while (EHStack.stable_begin() != Old) { @@ -432,14 +465,33 @@ PopCleanupBlock(FallThroughIsBranchThrough); } + + // Spill and reload any values for expressions that are required to + // dominate the fallthrough destination of an expression with cleanups. + for (llvm::Value **ReloadedValue : ValuesToReload) { + auto *Inst = cast(*ReloadedValue); + Address Tmp = + CreateDefaultAlignTempAlloca(Inst->getType(), "tmp.exprcleanup"); + + // Find an insertion point after Inst and spill it to the temporary. + llvm::BasicBlock::iterator InsertBefore; + if (auto *Invoke = dyn_cast(Inst)) + InsertBefore = Invoke->getNormalDest()->getFirstInsertionPt(); + else + InsertBefore = std::next(Inst->getIterator()); + CGBuilderTy(CGM, &*InsertBefore).CreateStore(Inst, Tmp); + + // Reload the value at the current insertion point. + *ReloadedValue = Builder.CreateLoad(Tmp); + } } /// Pops cleanup blocks until the given savepoint is reached, then add the /// cleanups from the given savepoint in the lifetime-extended cleanups stack. -void -CodeGenFunction::PopCleanupBlocks(EHScopeStack::stable_iterator Old, - size_t OldLifetimeExtendedSize) { - PopCleanupBlocks(Old); +void CodeGenFunction::PopCleanupBlocks( + EHScopeStack::stable_iterator Old, size_t OldLifetimeExtendedSize, + ArrayRef ValuesToReload) { + PopCleanupBlocks(Old, ValuesToReload); // Move our deferred cleanups onto the EH stack. for (size_t I = OldLifetimeExtendedSize, @@ -749,8 +801,8 @@ if (!HasPrebranchedFallthrough) Builder.CreateStore(Builder.getInt32(0), getNormalCleanupDestSlot()); - // Otherwise, save and clear the IP if we don't have fallthrough - // because the cleanup is inactive. + // Otherwise, save and clear the IP if we don't have fallthrough + // because the cleanup is inactive. } else if (FallthroughSource) { assert(!IsActive && "source without fallthrough for active cleanup"); savedInactiveFallthroughIP = Builder.saveAndClearIP(); @@ -891,8 +943,8 @@ assert(!FallthroughIsBranchThrough); EmitBlock(FallthroughDest); - // Case 3: a fallthrough source exists and should branch to the - // cleanup and then through to the next. + // Case 3: a fallthrough source exists and should branch to the + // cleanup and then through to the next. } else if (HasFallthrough) { // Everything is already set up for this. Index: lib/CodeGen/CGExprComplex.cpp =================================================================== --- lib/CodeGen/CGExprComplex.cpp +++ lib/CodeGen/CGExprComplex.cpp @@ -198,7 +198,13 @@ ComplexPairTy VisitExprWithCleanups(ExprWithCleanups *E) { CGF.enterFullExpression(E); CodeGenFunction::RunCleanupsScope Scope(CGF); - return Visit(E->getSubExpr()); + ComplexPairTy Vals = Visit(E->getSubExpr()); + // Defend against dominance problems caused by jumps out of expression + // evaluation through the shared cleanup block. + Scope.ensureDominatingValue(&Vals.first); + Scope.ensureDominatingValue(&Vals.second); + Scope.ForceCleanup(); + return Vals; } ComplexPairTy VisitCXXScalarValueInitExpr(CXXScalarValueInitExpr *E) { assert(E->getType()->isAnyComplexType() && "Expected complex type!"); Index: lib/CodeGen/CGExprScalar.cpp =================================================================== --- lib/CodeGen/CGExprScalar.cpp +++ lib/CodeGen/CGExprScalar.cpp @@ -12,6 +12,7 @@ //===----------------------------------------------------------------------===// #include "CodeGenFunction.h" +#include "CGCleanup.h" #include "CGCXXABI.h" #include "CGDebugInfo.h" #include "CGObjCRuntime.h" @@ -477,11 +478,7 @@ return CGF.LoadCXXThis(); } - Value *VisitExprWithCleanups(ExprWithCleanups *E) { - CGF.enterFullExpression(E); - CodeGenFunction::RunCleanupsScope Scope(CGF); - return Visit(E->getSubExpr()); - } + Value *VisitExprWithCleanups(ExprWithCleanups *E); Value *VisitCXXNewExpr(const CXXNewExpr *E) { return CGF.EmitCXXNewExpr(E); } @@ -1691,6 +1688,17 @@ E->getExprLoc()); } +Value *ScalarExprEmitter::VisitExprWithCleanups(ExprWithCleanups *E) { + CGF.enterFullExpression(E); + CodeGenFunction::RunCleanupsScope Scope(CGF); + Value *V = Visit(E->getSubExpr()); + // Defend against dominance problems caused by jumps out of expression + // evaluation through the shared cleanup block. + Scope.ensureDominatingValue(&V); + Scope.ForceCleanup(); + return V; +} + //===----------------------------------------------------------------------===// // Unary Operators //===----------------------------------------------------------------------===// Index: lib/CodeGen/CodeGenFunction.h =================================================================== --- lib/CodeGen/CodeGenFunction.h +++ lib/CodeGen/CodeGenFunction.h @@ -536,6 +536,7 @@ protected: bool PerformCleanup; private: + SmallVector ValuesToReload; RunCleanupsScope(const RunCleanupsScope &) = delete; void operator=(const RunCleanupsScope &) = delete; @@ -555,14 +556,10 @@ CGF.DidCallStackSave = false; } - /// \brief Exit this cleanup scope, emitting any accumulated - /// cleanups. + /// \brief Exit this cleanup scope, emitting any accumulated cleanups. ~RunCleanupsScope() { - if (PerformCleanup) { - CGF.DidCallStackSave = OldDidCallStackSave; - CGF.PopCleanupBlocks(CleanupStackDepth, - LifetimeExtendedCleanupStackSize); - } + if (PerformCleanup) + ForceCleanup(); } /// \brief Determine whether this scope requires any cleanups. @@ -570,13 +567,22 @@ return CGF.EHStack.stable_begin() != CleanupStackDepth; } + /// Ensure that *VP dominates the fallthrough insert point after cleanups + /// have run. Abnormal exits (break, return, etc) from GNU statement + /// expressions create the possibility that the insert point during + /// expression evaluation may not dominate the fallthrough insertion point + /// after running cleanups. Callers that wish to use values live across + /// cleanups can use this method to defend against this without creating + /// temporaries in the common case. + void ensureDominatingValue(llvm::Value **VP); + /// \brief Force the emission of cleanups now, instead of waiting /// until this object is destroyed. void ForceCleanup() { assert(PerformCleanup && "Already forced cleanup"); CGF.DidCallStackSave = OldDidCallStackSave; - CGF.PopCleanupBlocks(CleanupStackDepth, - LifetimeExtendedCleanupStackSize); + CGF.PopCleanupBlocks(CleanupStackDepth, LifetimeExtendedCleanupStackSize, + ValuesToReload); PerformCleanup = false; } }; @@ -738,13 +744,15 @@ /// \brief Takes the old cleanup stack size and emits the cleanup blocks /// that have been added. - void PopCleanupBlocks(EHScopeStack::stable_iterator OldCleanupStackSize); + void PopCleanupBlocks(EHScopeStack::stable_iterator OldCleanupStackSize, + ArrayRef ValuesToReload = None); /// \brief Takes the old cleanup stack size and emits the cleanup blocks /// that have been added, then adds all lifetime-extended cleanups from /// the given position to the stack. void PopCleanupBlocks(EHScopeStack::stable_iterator OldCleanupStackSize, - size_t OldLifetimeExtendedStackSize); + size_t OldLifetimeExtendedStackSize, + ArrayRef ValuesToReload); void ResolveBranchFixups(llvm::BasicBlock *Target); Index: test/CodeGenCXX/stmtexpr.cpp =================================================================== --- test/CodeGenCXX/stmtexpr.cpp +++ test/CodeGenCXX/stmtexpr.cpp @@ -80,3 +80,64 @@ y = ({ A a(1); if (b) goto G; a.i; }); G: return y; } + +// When we emit a full expression with cleanups that contains branches out of +// the full expression, the result of the inner expression (the call to +// call_with_cleanups in this case) may not dominate the fallthrough destination +// of the shared cleanup block. +// +// In this case the CFG will be a sequence of two diamonds, but the only +// dynamically possible execution paths are both left hand branches and both +// right hand branches. The first diamond LHS will call bar, and the second +// diamond LHS will assign the result to v, but the call to bar does not +// dominate the assignment. +int bar(A, int); +extern "C" int full_expr_branch(bool b) { + int v = bar(A(1), ({ if (b) return 42; 13; })); + return v; +} + +// CHECK-LABEL: define i32 @full_expr_branch({{.*}}) +// CHECK: call {{.*}} @_ZN1AC1Ei +// Spill after bar. +// CHECK: %[[v:[^ ]*]] = call i32 @_Z3bar1Ai({{.*}}) +// CHECK-NEXT: store i32 %[[v]], i32* %[[tmp:[^, ]*]] +// Do cleanup. +// CHECK: call {{.*}} @_ZN1AD1Ev +// CHECK: switch +// Reload before v assignment. +// CHECK: %[[v:[^ ]*]] = load i32, i32* %[[tmp]] +// CHECK-NEXT: store i32 %[[v]], i32* %v + +// We handle ExprWithCleanups for complex evaluation type separately, and it had +// the same bug. +_Complex float bar_complex(A, int); +extern "C" int full_expr_branch_complex(bool b) { + _Complex float v = bar_complex(A(1), ({ if (b) return 42; 13; })); + return v; +} + +// CHECK-LABEL: define i32 @full_expr_branch_complex({{.*}}) +// CHECK: call {{.*}} @_ZN1AC1Ei +// Spill after bar. +// CHECK: call {{.*}} @_Z11bar_complex1Ai({{.*}}) +// CHECK: store float %{{.*}}, float* %[[tmp1:[^, ]*]] +// CHECK: store float %{{.*}}, float* %[[tmp2:[^, ]*]] +// Do cleanup. +// CHECK: call {{.*}} @_ZN1AD1Ev +// CHECK: switch +// Reload before v assignment. +// CHECK: %[[v1:[^ ]*]] = load float, float* %[[tmp1]] +// CHECK: %[[v2:[^ ]*]] = load float, float* %[[tmp2]] +// CHECK: store float %[[v1]], float* %v.realp +// CHECK: store float %[[v2]], float* %v.imagp + +// No need to spill when the expression result is a constant, constants don't +// have dominance problems. +extern "C" int full_expr_branch_constant(bool b) { + int v = (A(1), (void)({ if (b) return 42; 0; }), 13); + return v; +} + +// CHECK-LABEL: define i32 @full_expr_branch_constant({{.*}}) +// CHECK: store i32 13, i32* %v