<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 6 March 2017 at 14:18, Reid Kleckner via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: rnk<br>
Date: Mon Mar 6 16:18:34 2017<br>
New Revision: 297084<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=297084&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=297084&view=rev</a><br>
Log:<br>
Don't assume cleanup emission preserves dominance in expr evaluation<br>
<br>
Summary:<br>
Because of the existence branches out of GNU statement expressions, it<br>
is possible that emitting cleanups for a full expression may cause the<br>
new insertion point to not be dominated by the result of the inner<br>
expression. Consider this example:<br>
<br>
struct Foo { Foo(); ~Foo(); int x; };<br>
int g(Foo, int);<br>
int f(bool cond) {<br>
int n = g(Foo(), ({ if (cond) return 0; 42; }));<br>
return n;<br>
}<br>
<br>
Before this change, result of the call to 'g' did not dominate its use<br>
in the store to 'n'. The early return exit from the statement expression<br>
branches to a shared cleanup block, which ends in a switch between the<br>
fallthrough destination (the assignment to 'n') or the function exit<br>
block.<br>
<br>
This change solves the problem by spilling and reloading expression<br>
evaluation results when any of the active cleanups have branches.<br>
<br>
I audited the other call sites of enterFullExpression, and they don't<br>
appear to keep and Values live across the site of the cleanup, except in<br>
ARC code. I wasn't able to create a test case for ARC that exhibits this<br>
problem, though.<br>
<br>
Reviewers: rjmccall, rsmith<br>
<br>
Subscribers: cfe-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D30590" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D30590</a><br>
<br>
Modified:<br>
cfe/trunk/lib/CodeGen/<wbr>CGCleanup.cpp<br>
cfe/trunk/lib/CodeGen/CGExpr.<wbr>cpp<br>
cfe/trunk/lib/CodeGen/<wbr>CGExprComplex.cpp<br>
cfe/trunk/lib/CodeGen/<wbr>CGExprScalar.cpp<br>
cfe/trunk/lib/CodeGen/<wbr>CodeGenFunction.h<br>
cfe/trunk/test/CodeGenCXX/<wbr>stmtexpr.cpp<br>
<br>
Modified: cfe/trunk/lib/CodeGen/<wbr>CGCleanup.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCleanup.cpp?rev=297084&r1=297083&r2=297084&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/CodeGen/<wbr>CGCleanup.cpp?rev=297084&r1=<wbr>297083&r2=297084&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/CodeGen/<wbr>CGCleanup.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/<wbr>CGCleanup.cpp Mon Mar 6 16:18:34 2017<br>
@@ -418,11 +418,15 @@ void CodeGenFunction::<wbr>ResolveBranchFixup<br>
}<br>
<br>
/// Pops cleanup blocks until the given savepoint is reached.<br>
-void CodeGenFunction::<wbr>PopCleanupBlocks(EHScopeStack:<wbr>:stable_iterator Old) {<br>
+void CodeGenFunction::<wbr>PopCleanupBlocks(<br>
+ EHScopeStack::stable_iterator Old,<br>
+ std::initializer_list<llvm::<wbr>Value **> ValuesToReload) {<br>
assert(Old.isValid());<br>
<br>
+ bool HadBranches = false;<br>
while (EHStack.stable_begin() != Old) {<br>
EHCleanupScope &Scope = cast<EHCleanupScope>(*EHStack.<wbr>begin());<br>
+ HadBranches |= Scope.hasBranches();<br>
<br>
// As long as Old strictly encloses the scope's enclosing normal<br>
// cleanup, we're going to emit another normal cleanup which<br>
@@ -432,14 +436,41 @@ void CodeGenFunction::<wbr>PopCleanupBlocks(E<br>
<br>
PopCleanupBlock(<wbr>FallThroughIsBranchThrough);<br>
}<br>
+<br>
+ // If we didn't have any branches, the insertion point before cleanups must<br>
+ // dominate the current insertion point and we don't need to reload any<br>
+ // values.<br>
+ if (!HadBranches)<br>
+ return;<br>
+<br>
+ // Spill and reload all values that the caller wants to be live at the current<br>
+ // insertion point.<br>
+ for (llvm::Value **ReloadedValue : ValuesToReload) {<br>
+ auto *Inst = dyn_cast_or_null<llvm::<wbr>Instruction>(*ReloadedValue);<br>
+ if (!Inst)<br>
+ continue;<br>
+ Address Tmp =<br>
+ CreateDefaultAlignTempAlloca(<wbr>Inst->getType(), "tmp.exprcleanup");<br>
+<br>
+ // Find an insertion point after Inst and spill it to the temporary.<br>
+ llvm::BasicBlock::iterator InsertBefore;<br>
+ if (auto *Invoke = dyn_cast<llvm::InvokeInst>(<wbr>Inst))<br>
+ InsertBefore = Invoke->getNormalDest()-><wbr>getFirstInsertionPt();<br>
+ else<br>
+ InsertBefore = std::next(Inst->getIterator())<wbr>;<br>
+ CGBuilderTy(CGM, &*InsertBefore).CreateStore(<wbr>Inst, Tmp);<br>
+<br>
+ // Reload the value at the current insertion point.<br>
+ *ReloadedValue = Builder.CreateLoad(Tmp);<br>
+ }<br>
}<br>
<br>
/// Pops cleanup blocks until the given savepoint is reached, then add the<br>
/// cleanups from the given savepoint in the lifetime-extended cleanups stack.<br>
-void<br>
-CodeGenFunction::<wbr>PopCleanupBlocks(EHScopeStack:<wbr>:stable_iterator Old,<br>
- size_t OldLifetimeExtendedSize) {<br>
- PopCleanupBlocks(Old);<br>
+void CodeGenFunction::<wbr>PopCleanupBlocks(<br>
+ EHScopeStack::stable_iterator Old, size_t OldLifetimeExtendedSize,<br>
+ std::initializer_list<llvm::<wbr>Value **> ValuesToReload) {<br>
+ PopCleanupBlocks(Old, ValuesToReload);<br>
<br>
// Move our deferred cleanups onto the EH stack.<br>
for (size_t I = OldLifetimeExtendedSize,<br>
<br>
Modified: cfe/trunk/lib/CodeGen/CGExpr.<wbr>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=297084&r1=297083&r2=297084&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/CodeGen/<wbr>CGExpr.cpp?rev=297084&r1=<wbr>297083&r2=297084&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/CodeGen/CGExpr.<wbr>cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/CGExpr.<wbr>cpp Mon Mar 6 16:18:34 2017<br>
@@ -1069,7 +1069,19 @@ LValue CodeGenFunction::EmitLValue(<wbr>const<br>
const auto *cleanups = cast<ExprWithCleanups>(E);<br>
enterFullExpression(cleanups);<br>
RunCleanupsScope Scope(*this);<br>
- return EmitLValue(cleanups-><wbr>getSubExpr());<br>
+ LValue LV = EmitLValue(cleanups-><wbr>getSubExpr());<br>
+ if (LV.isSimple()) {<br>
+ // Defend against branches out of gnu statement expressions surrounded by<br>
+ // cleanups.<br>
+ llvm::Value *V = LV.getPointer();<br>
+ Scope.ForceCleanup({&V});<br>
+ return LValue::MakeAddr(Address(V, LV.getAlignment()), LV.getType(),<br>
+ getContext(), LV.getAlignmentSource(),<br>
+ LV.getTBAAInfo());<br>
+ }<br>
+ // FIXME: Is it possible to create an ExprWithCleanups that produces a<br>
+ // bitfield lvalue or some other non-simple lvalue?<br></blockquote><div><br></div><div>Yes:</div><div><br></div><div><div>typedef __attribute__((ext_vector_type(4))) float v4f;</div><div>struct A { ~A(); };</div><div>void f(bool b, v4f v) { float &r = (A(), ({ if (b) return; }), v.x); }</div></div><div><br></div><div>However, IR generation asserts before it even gets here in that case :-(</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ return LV;<br>
}<br>
<br>
case Expr::CXXDefaultArgExprClass:<br>
<br>
Modified: cfe/trunk/lib/CodeGen/<wbr>CGExprComplex.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprComplex.cpp?rev=297084&r1=297083&r2=297084&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/CodeGen/<wbr>CGExprComplex.cpp?rev=297084&<wbr>r1=297083&r2=297084&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/CodeGen/<wbr>CGExprComplex.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/<wbr>CGExprComplex.cpp Mon Mar 6 16:18:34 2017<br>
@@ -198,7 +198,11 @@ public:<br>
ComplexPairTy VisitExprWithCleanups(<wbr>ExprWithCleanups *E) {<br>
CGF.enterFullExpression(E);<br>
CodeGenFunction::<wbr>RunCleanupsScope Scope(CGF);<br>
- return Visit(E->getSubExpr());<br>
+ ComplexPairTy Vals = Visit(E->getSubExpr());<br>
+ // Defend against dominance problems caused by jumps out of expression<br>
+ // evaluation through the shared cleanup block.<br>
+ Scope.ForceCleanup({&Vals.<wbr>first, &Vals.second});<br>
+ return Vals;<br>
}<br>
ComplexPairTy VisitCXXScalarValueInitExpr(<wbr>CXXScalarValueInitExpr *E) {<br>
assert(E->getType()-><wbr>isAnyComplexType() && "Expected complex type!");<br>
<br>
Modified: cfe/trunk/lib/CodeGen/<wbr>CGExprScalar.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=297084&r1=297083&r2=297084&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/CodeGen/<wbr>CGExprScalar.cpp?rev=297084&<wbr>r1=297083&r2=297084&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/CodeGen/<wbr>CGExprScalar.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/<wbr>CGExprScalar.cpp Mon Mar 6 16:18:34 2017<br>
@@ -12,6 +12,7 @@<br>
//===-------------------------<wbr>------------------------------<wbr>---------------===//<br>
<br>
#include "CodeGenFunction.h"<br>
+#include "CGCleanup.h"<br>
#include "CGCXXABI.h"<br>
#include "CGDebugInfo.h"<br>
#include "CGObjCRuntime.h"<br>
@@ -477,11 +478,7 @@ public:<br>
return CGF.LoadCXXThis();<br>
}<br>
<br>
- Value *VisitExprWithCleanups(<wbr>ExprWithCleanups *E) {<br>
- CGF.enterFullExpression(E);<br>
- CodeGenFunction::<wbr>RunCleanupsScope Scope(CGF);<br>
- return Visit(E->getSubExpr());<br>
- }<br>
+ Value *VisitExprWithCleanups(<wbr>ExprWithCleanups *E);<br>
Value *VisitCXXNewExpr(const CXXNewExpr *E) {<br>
return CGF.EmitCXXNewExpr(E);<br>
}<br>
@@ -1691,6 +1688,16 @@ Value *ScalarExprEmitter::<wbr>VisitStmtExpr(<br>
E->getExprLoc());<br>
}<br>
<br>
+Value *ScalarExprEmitter::<wbr>VisitExprWithCleanups(<wbr>ExprWithCleanups *E) {<br>
+ CGF.enterFullExpression(E);<br>
+ CodeGenFunction::<wbr>RunCleanupsScope Scope(CGF);<br>
+ Value *V = Visit(E->getSubExpr());<br>
+ // Defend against dominance problems caused by jumps out of expression<br>
+ // evaluation through the shared cleanup block.<br>
+ Scope.ForceCleanup({&V});<br>
+ return V;<br>
+}<br>
+<br>
//===-------------------------<wbr>------------------------------<wbr>---------------===//<br>
// Unary Operators<br>
//===-------------------------<wbr>------------------------------<wbr>---------------===//<br>
<br>
Modified: cfe/trunk/lib/CodeGen/<wbr>CodeGenFunction.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=297084&r1=297083&r2=297084&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/CodeGen/<wbr>CodeGenFunction.h?rev=297084&<wbr>r1=297083&r2=297084&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/CodeGen/<wbr>CodeGenFunction.h (original)<br>
+++ cfe/trunk/lib/CodeGen/<wbr>CodeGenFunction.h Mon Mar 6 16:18:34 2017<br>
@@ -580,14 +580,10 @@ public:<br>
CGF.DidCallStackSave = false;<br>
}<br>
<br>
- /// \brief Exit this cleanup scope, emitting any accumulated<br>
- /// cleanups.<br>
+ /// \brief Exit this cleanup scope, emitting any accumulated cleanups.<br>
~RunCleanupsScope() {<br>
- if (PerformCleanup) {<br>
- CGF.DidCallStackSave = OldDidCallStackSave;<br>
- CGF.PopCleanupBlocks(<wbr>CleanupStackDepth,<br>
- LifetimeExtendedCleanupStackSi<wbr>ze);<br>
- }<br>
+ if (PerformCleanup)<br>
+ ForceCleanup();<br>
}<br>
<br>
/// \brief Determine whether this scope requires any cleanups.<br>
@@ -597,11 +593,15 @@ public:<br>
<br>
/// \brief Force the emission of cleanups now, instead of waiting<br>
/// until this object is destroyed.<br>
- void ForceCleanup() {<br>
+ /// \param ValuesToReload - A list of values that need to be available at<br>
+ /// the insertion point after cleanup emission. If cleanup emission created<br>
+ /// a shared cleanup block, these value pointers will be rewritten.<br>
+ /// Otherwise, they not will be modified.<br>
+ void ForceCleanup(std::initializer_<wbr>list<llvm::Value**> ValuesToReload = {}) {<br>
assert(PerformCleanup && "Already forced cleanup");<br>
CGF.DidCallStackSave = OldDidCallStackSave;<br>
- CGF.PopCleanupBlocks(<wbr>CleanupStackDepth,<br>
- LifetimeExtendedCleanupStackSi<wbr>ze);<br>
+ CGF.PopCleanupBlocks(<wbr>CleanupStackDepth, LifetimeExtendedCleanupStackSi<wbr>ze,<br>
+ ValuesToReload);<br>
PerformCleanup = false;<br>
}<br>
};<br>
@@ -763,13 +763,17 @@ public:<br>
<br>
/// \brief Takes the old cleanup stack size and emits the cleanup blocks<br>
/// that have been added.<br>
- void PopCleanupBlocks(EHScopeStack:<wbr>:stable_iterator OldCleanupStackSize);<br>
+ void<br>
+ PopCleanupBlocks(EHScopeStack:<wbr>:stable_iterator OldCleanupStackSize,<br>
+ std::initializer_list<llvm::<wbr>Value **> ValuesToReload = {});<br>
<br>
/// \brief Takes the old cleanup stack size and emits the cleanup blocks<br>
/// that have been added, then adds all lifetime-extended cleanups from<br>
/// the given position to the stack.<br>
- void PopCleanupBlocks(EHScopeStack:<wbr>:stable_iterator OldCleanupStackSize,<br>
- size_t OldLifetimeExtendedStackSize);<br>
+ void<br>
+ PopCleanupBlocks(EHScopeStack:<wbr>:stable_iterator OldCleanupStackSize,<br>
+ size_t OldLifetimeExtendedStackSize,<br>
+ std::initializer_list<llvm::<wbr>Value **> ValuesToReload = {});<br>
<br>
void ResolveBranchFixups(llvm::<wbr>BasicBlock *Target);<br>
<br>
<br>
Modified: cfe/trunk/test/CodeGenCXX/<wbr>stmtexpr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/stmtexpr.cpp?rev=297084&r1=297083&r2=297084&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/<wbr>CodeGenCXX/stmtexpr.cpp?rev=<wbr>297084&r1=297083&r2=297084&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/CodeGenCXX/<wbr>stmtexpr.cpp (original)<br>
+++ cfe/trunk/test/CodeGenCXX/<wbr>stmtexpr.cpp Mon Mar 6 16:18:34 2017<br>
@@ -80,3 +80,85 @@ int foo5(bool b) {<br>
y = ({ A a(1); if (b) goto G; a.i; });<br>
G: return y;<br>
}<br>
+<br>
+// When we emit a full expression with cleanups that contains branches out of<br>
+// the full expression, the result of the inner expression (the call to<br>
+// call_with_cleanups in this case) may not dominate the fallthrough destination<br>
+// of the shared cleanup block.<br>
+//<br>
+// In this case the CFG will be a sequence of two diamonds, but the only<br>
+// dynamically possible execution paths are both left hand branches and both<br>
+// right hand branches. The first diamond LHS will call bar, and the second<br>
+// diamond LHS will assign the result to v, but the call to bar does not<br>
+// dominate the assignment.<br>
+int bar(A, int);<br>
+extern "C" int cleanup_exit_scalar(bool b) {<br>
+ int v = bar(A(1), ({ if (b) return 42; 13; }));<br>
+ return v;<br>
+}<br>
+<br>
+// CHECK-LABEL: define i32 @cleanup_exit_scalar({{.*}})<br>
+// CHECK: call {{.*}} @_ZN1AC1Ei<br>
+// Spill after bar.<br>
+// CHECK: %[[v:[^ ]*]] = call i32 @_Z3bar1Ai({{.*}})<br>
+// CHECK-NEXT: store i32 %[[v]], i32* %[[tmp:[^, ]*]]<br>
+// Do cleanup.<br>
+// CHECK: call {{.*}} @_ZN1AD1Ev<br>
+// CHECK: switch<br>
+// Reload before v assignment.<br>
+// CHECK: %[[v:[^ ]*]] = load i32, i32* %[[tmp]]<br>
+// CHECK-NEXT: store i32 %[[v]], i32* %v<br>
+<br>
+// No need to spill when the expression result is a constant, constants don't<br>
+// have dominance problems.<br>
+extern "C" int cleanup_exit_scalar_constant(<wbr>bool b) {<br>
+ int v = (A(1), (void)({ if (b) return 42; 0; }), 13);<br>
+ return v;<br>
+}<br>
+<br>
+// CHECK-LABEL: define i32 @cleanup_exit_scalar_constant(<wbr>{{.*}})<br>
+// CHECK: store i32 13, i32* %v<br>
+<br>
+// Check for the same bug for lvalue expression evaluation kind.<br>
+// FIXME: What about non-reference lvalues, like bitfield lvalues and vector<br>
+// lvalues?<br>
+int &getref();<br>
+extern "C" int cleanup_exit_lvalue(bool cond) {<br>
+ int &r = (A(1), ({ if (cond) return 0; (void)0; }), getref());<br>
+ return r;<br>
+}<br>
+// CHECK-LABEL: define i32 @cleanup_exit_lvalue({{.*}})<br>
+// CHECK: call {{.*}} @_ZN1AC1Ei<br>
+// Spill after bar.<br>
+// CHECK: %[[v:[^ ]*]] = call dereferenceable(4) i32* @_Z6getrefv({{.*}})<br>
+// CHECK-NEXT: store i32* %[[v]], i32** %[[tmp:[^, ]*]]<br>
+// Do cleanup.<br>
+// CHECK: call {{.*}} @_ZN1AD1Ev<br>
+// CHECK: switch<br>
+// Reload before v assignment.<br>
+// CHECK: %[[v:[^ ]*]] = load i32*, i32** %[[tmp]]<br>
+// CHECK-NEXT: store i32* %[[v]], i32** %r<br>
+<br>
+<br>
+// We handle ExprWithCleanups for complex evaluation type separately, and it had<br>
+// the same bug.<br>
+_Complex float bar_complex(A, int);<br>
+extern "C" int cleanup_exit_complex(bool b) {<br>
+ _Complex float v = bar_complex(A(1), ({ if (b) return 42; 13; }));<br>
+ return v;<br>
+}<br>
+<br>
+// CHECK-LABEL: define i32 @cleanup_exit_complex({{.*}})<br>
+// CHECK: call {{.*}} @_ZN1AC1Ei<br>
+// Spill after bar.<br>
+// CHECK: call {{.*}} @_Z11bar_complex1Ai({{.*}})<br>
+// CHECK: store float %{{.*}}, float* %[[tmp1:[^, ]*]]<br>
+// CHECK: store float %{{.*}}, float* %[[tmp2:[^, ]*]]<br>
+// Do cleanup.<br>
+// CHECK: call {{.*}} @_ZN1AD1Ev<br>
+// CHECK: switch<br>
+// Reload before v assignment.<br>
+// CHECK: %[[v1:[^ ]*]] = load float, float* %[[tmp1]]<br>
+// CHECK: %[[v2:[^ ]*]] = load float, float* %[[tmp2]]<br>
+// CHECK: store float %[[v1]], float* %v.realp<br>
+// CHECK: store float %[[v2]], float* %v.imagp<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>