Index: cfe/trunk/lib/CodeGen/CGCleanup.cpp =================================================================== --- cfe/trunk/lib/CodeGen/CGCleanup.cpp +++ cfe/trunk/lib/CodeGen/CGCleanup.cpp @@ -418,11 +418,15 @@ } /// Pops cleanup blocks until the given savepoint is reached. -void CodeGenFunction::PopCleanupBlocks(EHScopeStack::stable_iterator Old) { +void CodeGenFunction::PopCleanupBlocks( + EHScopeStack::stable_iterator Old, + std::initializer_list ValuesToReload) { assert(Old.isValid()); + bool HadBranches = false; while (EHStack.stable_begin() != Old) { EHCleanupScope &Scope = cast(*EHStack.begin()); + HadBranches |= Scope.hasBranches(); // As long as Old strictly encloses the scope's enclosing normal // cleanup, we're going to emit another normal cleanup which @@ -432,14 +436,41 @@ PopCleanupBlock(FallThroughIsBranchThrough); } + + // If we didn't have any branches, the insertion point before cleanups must + // dominate the current insertion point and we don't need to reload any + // values. + if (!HadBranches) + return; + + // Spill and reload all values that the caller wants to be live at the current + // insertion point. + for (llvm::Value **ReloadedValue : ValuesToReload) { + auto *Inst = dyn_cast_or_null(*ReloadedValue); + if (!Inst) + continue; + 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, + std::initializer_list ValuesToReload) { + PopCleanupBlocks(Old, ValuesToReload); // Move our deferred cleanups onto the EH stack. for (size_t I = OldLifetimeExtendedSize, Index: cfe/trunk/lib/CodeGen/CGExpr.cpp =================================================================== --- cfe/trunk/lib/CodeGen/CGExpr.cpp +++ cfe/trunk/lib/CodeGen/CGExpr.cpp @@ -1069,7 +1069,19 @@ const auto *cleanups = cast(E); enterFullExpression(cleanups); RunCleanupsScope Scope(*this); - return EmitLValue(cleanups->getSubExpr()); + LValue LV = EmitLValue(cleanups->getSubExpr()); + if (LV.isSimple()) { + // Defend against branches out of gnu statement expressions surrounded by + // cleanups. + llvm::Value *V = LV.getPointer(); + Scope.ForceCleanup({&V}); + return LValue::MakeAddr(Address(V, LV.getAlignment()), LV.getType(), + getContext(), LV.getAlignmentSource(), + LV.getTBAAInfo()); + } + // FIXME: Is it possible to create an ExprWithCleanups that produces a + // bitfield lvalue or some other non-simple lvalue? + return LV; } case Expr::CXXDefaultArgExprClass: Index: cfe/trunk/lib/CodeGen/CGExprComplex.cpp =================================================================== --- cfe/trunk/lib/CodeGen/CGExprComplex.cpp +++ cfe/trunk/lib/CodeGen/CGExprComplex.cpp @@ -198,7 +198,11 @@ 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.ForceCleanup({&Vals.first, &Vals.second}); + return Vals; } ComplexPairTy VisitCXXScalarValueInitExpr(CXXScalarValueInitExpr *E) { assert(E->getType()->isAnyComplexType() && "Expected complex type!"); Index: cfe/trunk/lib/CodeGen/CGExprScalar.cpp =================================================================== --- cfe/trunk/lib/CodeGen/CGExprScalar.cpp +++ cfe/trunk/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,16 @@ 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.ForceCleanup({&V}); + return V; +} + //===----------------------------------------------------------------------===// // Unary Operators //===----------------------------------------------------------------------===// Index: cfe/trunk/lib/CodeGen/CodeGenFunction.h =================================================================== --- cfe/trunk/lib/CodeGen/CodeGenFunction.h +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h @@ -580,14 +580,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. @@ -597,11 +593,15 @@ /// \brief Force the emission of cleanups now, instead of waiting /// until this object is destroyed. - void ForceCleanup() { + /// \param ValuesToReload - A list of values that need to be available at + /// the insertion point after cleanup emission. If cleanup emission created + /// a shared cleanup block, these value pointers will be rewritten. + /// Otherwise, they not will be modified. + void ForceCleanup(std::initializer_list ValuesToReload = {}) { assert(PerformCleanup && "Already forced cleanup"); CGF.DidCallStackSave = OldDidCallStackSave; - CGF.PopCleanupBlocks(CleanupStackDepth, - LifetimeExtendedCleanupStackSize); + CGF.PopCleanupBlocks(CleanupStackDepth, LifetimeExtendedCleanupStackSize, + ValuesToReload); PerformCleanup = false; } }; @@ -763,13 +763,17 @@ /// \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, + std::initializer_list ValuesToReload = {}); /// \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); + void + PopCleanupBlocks(EHScopeStack::stable_iterator OldCleanupStackSize, + size_t OldLifetimeExtendedStackSize, + std::initializer_list ValuesToReload = {}); void ResolveBranchFixups(llvm::BasicBlock *Target); Index: cfe/trunk/test/CodeGenCXX/stmtexpr.cpp =================================================================== --- cfe/trunk/test/CodeGenCXX/stmtexpr.cpp +++ cfe/trunk/test/CodeGenCXX/stmtexpr.cpp @@ -80,3 +80,85 @@ 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 cleanup_exit_scalar(bool b) { + int v = bar(A(1), ({ if (b) return 42; 13; })); + return v; +} + +// CHECK-LABEL: define i32 @cleanup_exit_scalar({{.*}}) +// 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 + +// No need to spill when the expression result is a constant, constants don't +// have dominance problems. +extern "C" int cleanup_exit_scalar_constant(bool b) { + int v = (A(1), (void)({ if (b) return 42; 0; }), 13); + return v; +} + +// CHECK-LABEL: define i32 @cleanup_exit_scalar_constant({{.*}}) +// CHECK: store i32 13, i32* %v + +// Check for the same bug for lvalue expression evaluation kind. +// FIXME: What about non-reference lvalues, like bitfield lvalues and vector +// lvalues? +int &getref(); +extern "C" int cleanup_exit_lvalue(bool cond) { + int &r = (A(1), ({ if (cond) return 0; (void)0; }), getref()); + return r; +} +// CHECK-LABEL: define i32 @cleanup_exit_lvalue({{.*}}) +// CHECK: call {{.*}} @_ZN1AC1Ei +// Spill after bar. +// CHECK: %[[v:[^ ]*]] = call dereferenceable(4) i32* @_Z6getrefv({{.*}}) +// 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** %r + + +// We handle ExprWithCleanups for complex evaluation type separately, and it had +// the same bug. +_Complex float bar_complex(A, int); +extern "C" int cleanup_exit_complex(bool b) { + _Complex float v = bar_complex(A(1), ({ if (b) return 42; 13; })); + return v; +} + +// CHECK-LABEL: define i32 @cleanup_exit_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