On 6 March 2017 at 14:18, Reid Kleckner via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> Author: rnk > Date: Mon Mar 6 16:18:34 2017 > New Revision: 297084 > > URL: http://llvm.org/viewvc/llvm-project?rev=297084&view=rev > Log: > Don't assume cleanup emission preserves dominance in expr evaluation > > Summary: > Because of the existence branches out of GNU statement expressions, it > is possible that emitting cleanups for a full expression may cause the > new insertion point to not be dominated by the result of the inner > expression. Consider this example: > > struct Foo { Foo(); ~Foo(); int x; }; > int g(Foo, int); > int f(bool cond) { > int n = g(Foo(), ({ if (cond) return 0; 42; })); > return n; > } > > Before this change, result of the call to 'g' did not dominate its use > in the store to 'n'. The early return exit from the statement expression > branches to a shared cleanup block, which ends in a switch between the > fallthrough destination (the assignment to 'n') or the function exit > block. > > This change solves the problem by spilling and reloading expression > evaluation results when any of the active cleanups have branches. > > I audited the other call sites of enterFullExpression, and they don't > appear to keep and Values live across the site of the cleanup, except in > ARC code. I wasn't able to create a test case for ARC that exhibits this > problem, though. > > Reviewers: rjmccall, rsmith > > Subscribers: cfe-commits > > Differential Revision: https://reviews.llvm.org/D30590 > > Modified: > cfe/trunk/lib/CodeGen/CGCleanup.cpp > cfe/trunk/lib/CodeGen/CGExpr.cpp > cfe/trunk/lib/CodeGen/CGExprComplex.cpp > cfe/trunk/lib/CodeGen/CGExprScalar.cpp > cfe/trunk/lib/CodeGen/CodeGenFunction.h > cfe/trunk/test/CodeGenCXX/stmtexpr.cpp > > Modified: cfe/trunk/lib/CodeGen/CGCleanup.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ > CGCleanup.cpp?rev=297084&r1=297083&r2=297084&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/CodeGen/CGCleanup.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGCleanup.cpp Mon Mar 6 16:18:34 2017 > @@ -418,11 +418,15 @@ void CodeGenFunction::ResolveBranchFixup > } > > /// 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<llvm::Value **> ValuesToReload) { > assert(Old.isValid()); > > + bool HadBranches = false; > while (EHStack.stable_begin() != Old) { > EHCleanupScope &Scope = cast<EHCleanupScope>(*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 @@ void CodeGenFunction::PopCleanupBlocks(E > > 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<llvm::Instruction>(*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<llvm::InvokeInst>(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<llvm::Value **> ValuesToReload) { > + PopCleanupBlocks(Old, ValuesToReload); > > // Move our deferred cleanups onto the EH stack. > for (size_t I = OldLifetimeExtendedSize, > > Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ > CGExpr.cpp?rev=297084&r1=297083&r2=297084&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/CodeGen/CGExpr.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGExpr.cpp Mon Mar 6 16:18:34 2017 > @@ -1069,7 +1069,19 @@ LValue CodeGenFunction::EmitLValue(const > const auto *cleanups = cast<ExprWithCleanups>(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? > Yes: typedef __attribute__((ext_vector_type(4))) float v4f; struct A { ~A(); }; void f(bool b, v4f v) { float &r = (A(), ({ if (b) return; }), v.x); } However, IR generation asserts before it even gets here in that case :-( > + return LV; > } > > case Expr::CXXDefaultArgExprClass: > > Modified: cfe/trunk/lib/CodeGen/CGExprComplex.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ > CGExprComplex.cpp?rev=297084&r1=297083&r2=297084&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/CodeGen/CGExprComplex.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGExprComplex.cpp Mon Mar 6 16:18:34 2017 > @@ -198,7 +198,11 @@ public: > 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!"); > > Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ > CGExprScalar.cpp?rev=297084&r1=297083&r2=297084&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Mon Mar 6 16:18:34 2017 > @@ -12,6 +12,7 @@ > //===------------------------------------------------------- > ---------------===// > > #include "CodeGenFunction.h" > +#include "CGCleanup.h" > #include "CGCXXABI.h" > #include "CGDebugInfo.h" > #include "CGObjCRuntime.h" > @@ -477,11 +478,7 @@ public: > 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 @@ Value *ScalarExprEmitter::VisitStmtExpr( > 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 > //===------------------------------------------------------- > ---------------===// > > Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ > CodeGenFunction.h?rev=297084&r1=297083&r2=297084&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original) > +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Mon Mar 6 16:18:34 2017 > @@ -580,14 +580,10 @@ public: > 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 @@ public: > > /// \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<llvm::Value**> > ValuesToReload = {}) { > assert(PerformCleanup && "Already forced cleanup"); > CGF.DidCallStackSave = OldDidCallStackSave; > - CGF.PopCleanupBlocks(CleanupStackDepth, > - LifetimeExtendedCleanupStackSize); > + CGF.PopCleanupBlocks(CleanupStackDepth, > LifetimeExtendedCleanupStackSize, > + ValuesToReload); > PerformCleanup = false; > } > }; > @@ -763,13 +763,17 @@ public: > > /// \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<llvm::Value **> 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<llvm::Value **> ValuesToReload > = {}); > > void ResolveBranchFixups(llvm::BasicBlock *Target); > > > Modified: cfe/trunk/test/CodeGenCXX/stmtexpr.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ > CodeGenCXX/stmtexpr.cpp?rev=297084&r1=297083&r2=297084&view=diff > ============================================================ > ================== > --- cfe/trunk/test/CodeGenCXX/stmtexpr.cpp (original) > +++ cfe/trunk/test/CodeGenCXX/stmtexpr.cpp Mon Mar 6 16:18:34 2017 > @@ -80,3 +80,85 @@ int foo5(bool b) { > 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 > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
