<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>