r297084 - Don't assume cleanup emission preserves dominance in expr evaluation
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 6 15:06:22 PST 2017
On 6 March 2017 at 14:18, Reid Kleckner via cfe-commits <
cfe-commits at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170306/ab3faf34/attachment-0001.html>
More information about the cfe-commits
mailing list