<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 7, 2014 at 9:16 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Aug 7, 2014 at 11:44 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
> Author: klimek<br>
> Date: Thu Aug 7 13:44:19 2014<br>
> New Revision: 215128<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=215128&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=215128&view=rev</a><br>
> Log:<br>
> Mark successors as reachable/unreachable instead of changing the CFG.<br>
><br>
> As suggested by Ted, this makes a few warnings less aggressive.<br>
><br>
> Modified:<br>
> cfe/trunk/lib/Analysis/CFG.cpp<br>
> cfe/trunk/test/SemaCXX/return-noreturn.cpp<br>
><br>
> Modified: cfe/trunk/lib/Analysis/CFG.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=215128&r1=215127&r2=215128&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=215128&r1=215127&r2=215128&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Analysis/CFG.cpp (original)<br>
> +++ cfe/trunk/lib/Analysis/CFG.cpp Thu Aug 7 13:44:19 2014<br>
> @@ -479,6 +479,7 @@ private:<br>
> AbstractConditionalOperator *E, bool BindToTemporary,<br>
> TempDtorContext &Context);<br>
> void InsertTempDtorDecisionBlock(const TempDtorContext &Context,<br>
> + TryResult ConditionVal,<br>
> CFGBlock *FalseSucc = nullptr);<br>
><br>
> // NYS == Not Yet Supported<br>
> @@ -3631,24 +3632,16 @@ CFGBlock *CFGBuilder::VisitBinaryOperato<br>
> BinaryOperator *E, TempDtorContext &Context) {<br>
> if (E->isLogicalOp()) {<br>
> VisitForTemporaryDtors(E->getLHS(), false, Context);<br>
> - TryResult LHSVal = tryEvaluateBool(E->getLHS());<br>
> - bool RHSNotExecuted = (E->getOpcode() == BO_LAnd && LHSVal.isFalse()) ||<br>
> - (E->getOpcode() == BO_LOr && LHSVal.isTrue());<br>
> - if (RHSNotExecuted) {<br>
> - return Block;<br>
> - }<br>
> -<br>
> - // If the LHS is known, and the RHS is not executed, we returned above.<br>
> - // Thus, once we arrive here, and the LHS is known, we also know that the<br>
> - // RHS was executed and can execute the RHS unconditionally (that is, we<br>
> - // don't insert a decision block).<br>
> - if (LHSVal.isKnown()) {<br>
> - VisitForTemporaryDtors(E->getRHS(), false, Context);<br>
> - } else {<br>
> - TempDtorContext RHSContext(/*IsConditional=*/true);<br>
> - VisitForTemporaryDtors(E->getRHS(), false, RHSContext);<br>
> - InsertTempDtorDecisionBlock(RHSContext);<br>
> - }<br>
> + TryResult RHSExecuted = tryEvaluateBool(E->getLHS());<br>
> + if (RHSExecuted.isKnown() && E->getOpcode() == BO_LOr)<br>
> + RHSExecuted.negate();<br>
> +<br>
> + // We do not know at CFG-construction time whether the right-hand-side was<br>
> + // executed, thus we add a branch node that depends on the temporary<br>
> + // constructor call.<br>
> + TempDtorContext RHSContext(/*IsConditional=*/true);<br>
> + VisitForTemporaryDtors(E->getRHS(), false, RHSContext);<br>
> + InsertTempDtorDecisionBlock(RHSContext, RHSExecuted);<br>
><br>
> return Block;<br>
> }<br>
> @@ -3705,6 +3698,7 @@ CFGBlock *CFGBuilder::VisitCXXBindTempor<br>
> }<br>
><br>
> void CFGBuilder::InsertTempDtorDecisionBlock(const TempDtorContext &Context,<br>
> + TryResult ConditionVal,<br>
> CFGBlock *FalseSucc) {<br>
> if (!Context.TerminatorExpr) {<br>
> // If no temporary was found, we do not need to insert a decision point.<br>
> @@ -3713,8 +3707,9 @@ void CFGBuilder::InsertTempDtorDecisionB<br>
> assert(Context.TerminatorExpr);<br>
> CFGBlock *Decision = createBlock(false);<br>
> Decision->setTerminator(CFGTerminator(Context.TerminatorExpr, true));<br>
> - addSuccessor(Decision, Block);<br>
> - addSuccessor(Decision, FalseSucc ? FalseSucc : Context.Succ);<br>
> + addSuccessor(Decision, Block, !ConditionVal.isFalse());<br>
> + addSuccessor(Decision, FalseSucc ? FalseSucc : Context.Succ,<br>
> + !ConditionVal.isTrue());<br>
> Block = Decision;<br>
> }<br>
><br>
> @@ -3725,16 +3720,8 @@ CFGBlock *CFGBuilder::VisitConditionalOp<br>
> CFGBlock *ConditionBlock = Block;<br>
> CFGBlock *ConditionSucc = Succ;<br>
> TryResult ConditionVal = tryEvaluateBool(E->getCond());<br>
> -<br>
> - if (ConditionVal.isKnown()) {<br>
> - if (ConditionVal.isTrue()) {<br>
> - VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary, Context);<br>
> - } else {<br>
> - assert(ConditionVal.isFalse());<br>
> - VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary, Context);<br>
> - }<br>
> - return Block;<br>
> - }<br>
> + TryResult NegatedVal = ConditionVal;<br>
> + if (NegatedVal.isKnown()) NegatedVal.negate();<br>
><br>
> TempDtorContext TrueContext(/*IsConditional=*/true);<br>
> VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary, TrueContext);<br>
> @@ -3746,12 +3733,12 @@ CFGBlock *CFGBuilder::VisitConditionalOp<br>
> VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary, FalseContext);<br>
><br>
> if (TrueContext.TerminatorExpr && FalseContext.TerminatorExpr) {<br>
> - InsertTempDtorDecisionBlock(FalseContext, TrueBlock);<br>
> + InsertTempDtorDecisionBlock(FalseContext, NegatedVal, TrueBlock);<br>
> } else if (TrueContext.TerminatorExpr) {<br>
> Block = TrueBlock;<br>
> - InsertTempDtorDecisionBlock(TrueContext);<br>
> + InsertTempDtorDecisionBlock(TrueContext, ConditionVal);<br>
> } else {<br>
> - InsertTempDtorDecisionBlock(FalseContext);<br>
> + InsertTempDtorDecisionBlock(FalseContext, NegatedVal);<br>
> }<br>
> return Block;<br>
> }<br>
><br>
> Modified: cfe/trunk/test/SemaCXX/return-noreturn.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return-noreturn.cpp?rev=215128&r1=215127&r2=215128&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return-noreturn.cpp?rev=215128&r1=215127&r2=215128&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/SemaCXX/return-noreturn.cpp (original)<br>
> +++ cfe/trunk/test/SemaCXX/return-noreturn.cpp Thu Aug 7 13:44:19 2014<br>
> @@ -185,11 +185,11 @@ int testTernaryConditionalNoreturnFalseB<br>
><br>
> int testConditionallyExecutedComplexTernaryTrueBranch(bool value) {<br>
> value || (true ? NoReturn() : true);<br>
> -} // expected-warning {{control may reach end of non-void function}}<br>
> +}<br>
<br>
</div></div>I'm a bit confused as to why /this/ warning would go away when you add<br>
the optimstic/pessimistic edges. This warning should still fire, as,<br>
when 'value' is false, control does reach the end of a non-void<br>
function.<br></blockquote><div><br></div><div>Yea, I think the problem is that when we annotate a branch with reachability information, we have to always add a CFG branch at the next higher level branch point until we hit an unannotated branch. I'll add that tomorrow.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The warnings I would expect to not fire with the kind of change Ted<br>
was suggesting (and I don't know if this is a pre-existing problem, an<br>
issue with this patch, or an issue with my understanding) would be<br>
things like -Wunreachable-code, probably in cases like:<br>
<br>
true ? NoReturn() : true;<br>
func(); // -Wunreachable-code should fire here<br>
<br>
ARCH_CONST ? NoReturn() : true;<br>
func(); // don't warn that this is unreachable, because ARCH_CONST<br>
could vary by build<br>
<br>
Or something like that - I forget which particular heuristics Ted put<br>
in to suppress build-varying constants.<br>
<div class="HOEnZb"><div class="h5"><br>
> int testConditionallyExecutedComplexTernaryFalseBranch(bool value) {<br>
> value || (false ? true : NoReturn());<br>
> -} // expected-warning {{control may reach end of non-void function}}<br>
> +}<br>
><br>
> int testStaticallyExecutedLogicalOrBranch() {<br>
> false || NoReturn();<br>
> @@ -209,7 +209,7 @@ int testStaticallySkppedLogicalAndBranch<br>
><br>
> int testConditionallyExecutedComplexLogicalBranch(bool value) {<br>
> value || (true && NoReturn());<br>
> -} // expected-warning {{control may reach end of non-void function}}<br>
> +}<br>
><br>
> #if __cplusplus >= 201103L<br>
> namespace LambdaVsTemporaryDtor {<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>