rnk created this revision.
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.
https://reviews.llvm.org/D30590
Files:
lib/CodeGen/CGCleanup.cpp
lib/CodeGen/CGExprComplex.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenCXX/stmtexpr.cpp
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
Index: lib/CodeGen/CodeGenFunction.h
===================================================================
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -536,6 +536,7 @@
protected:
bool PerformCleanup;
private:
+ SmallVector<llvm::Value **, 1> ValuesToReload;
RunCleanupsScope(const RunCleanupsScope &) = delete;
void operator=(const RunCleanupsScope &) = delete;
@@ -555,28 +556,33 @@
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.
bool requiresCleanups() const {
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<llvm::Value **> 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<llvm::Value **> ValuesToReload);
void ResolveBranchFixups(llvm::BasicBlock *Target);
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/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/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<llvm::Instruction>(*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<EHCleanupScope>(*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<llvm::Value **> 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<llvm::Instruction>(*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<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,
+ ArrayRef<llvm::Value **> 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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
