This revision was automatically updated to reflect the committed changes.
Closed by commit rL297084: Don't assume cleanup emission preserves dominance in
expr evaluation (authored by rnk).
Changed prior to commit:
https://reviews.llvm.org/D30590?vs=90743&id=90747#toc
Repository:
rL LLVM
https://reviews.llvm.org/D30590
Files:
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
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
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<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 @@
/// \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);
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<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 @@
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,
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<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?
+ return LV;
}
case Expr::CXXDefaultArgExprClass:
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/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!");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
