r215128 - Mark successors as reachable/unreachable instead of changing the CFG.

[David Blaikie]

On Thu, Aug 7, 2014 at 11:44 AM, Manuel Klimek <klimek at google.com> wrote:
> Author: klimek
> Date: Thu Aug  7 13:44:19 2014
> New Revision: 215128
>
> URL: http://llvm.org/viewvc/llvm-project?rev=215128&view=rev
> Log:
> Mark successors as reachable/unreachable instead of changing the CFG.
>
> As suggested by Ted, this makes a few warnings less aggressive.
>
> Modified:
>     cfe/trunk/lib/Analysis/CFG.cpp
>     cfe/trunk/test/SemaCXX/return-noreturn.cpp
>
> Modified: cfe/trunk/lib/Analysis/CFG.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=215128&r1=215127&r2=215128&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Analysis/CFG.cpp (original)
> +++ cfe/trunk/lib/Analysis/CFG.cpp Thu Aug  7 13:44:19 2014
> @@ -479,6 +479,7 @@ private:
>        AbstractConditionalOperator *E, bool BindToTemporary,
>        TempDtorContext &Context);
>    void InsertTempDtorDecisionBlock(const TempDtorContext &Context,
> +                                   TryResult ConditionVal,
>                                     CFGBlock *FalseSucc = nullptr);
>
>    // NYS == Not Yet Supported
> @@ -3631,24 +3632,16 @@ CFGBlock *CFGBuilder::VisitBinaryOperato
>      BinaryOperator *E, TempDtorContext &Context) {
>    if (E->isLogicalOp()) {
>      VisitForTemporaryDtors(E->getLHS(), false, Context);
> -    TryResult LHSVal = tryEvaluateBool(E->getLHS());
> -    bool RHSNotExecuted = (E->getOpcode() == BO_LAnd && LHSVal.isFalse()) ||
> -                          (E->getOpcode() == BO_LOr && LHSVal.isTrue());
> -    if (RHSNotExecuted) {
> -      return Block;
> -    }
> -
> -    // If the LHS is known, and the RHS is not executed, we returned above.
> -    // Thus, once we arrive here, and the LHS is known, we also know that the
> -    // RHS was executed and can execute the RHS unconditionally (that is, we
> -    // don't insert a decision block).
> -    if (LHSVal.isKnown()) {
> -      VisitForTemporaryDtors(E->getRHS(), false, Context);
> -    } else {
> -      TempDtorContext RHSContext(/*IsConditional=*/true);
> -      VisitForTemporaryDtors(E->getRHS(), false, RHSContext);
> -      InsertTempDtorDecisionBlock(RHSContext);
> -    }
> +    TryResult RHSExecuted = tryEvaluateBool(E->getLHS());
> +    if (RHSExecuted.isKnown() && E->getOpcode() == BO_LOr)
> +      RHSExecuted.negate();
> +
> +    // We do not know at CFG-construction time whether the right-hand-side was
> +    // executed, thus we add a branch node that depends on the temporary
> +    // constructor call.
> +    TempDtorContext RHSContext(/*IsConditional=*/true);
> +    VisitForTemporaryDtors(E->getRHS(), false, RHSContext);
> +    InsertTempDtorDecisionBlock(RHSContext, RHSExecuted);
>
>      return Block;
>    }
> @@ -3705,6 +3698,7 @@ CFGBlock *CFGBuilder::VisitCXXBindTempor
>  }
>
>  void CFGBuilder::InsertTempDtorDecisionBlock(const TempDtorContext &Context,
> +                                             TryResult ConditionVal,
>                                               CFGBlock *FalseSucc) {
>    if (!Context.TerminatorExpr) {
>      // If no temporary was found, we do not need to insert a decision point.
> @@ -3713,8 +3707,9 @@ void CFGBuilder::InsertTempDtorDecisionB
>    assert(Context.TerminatorExpr);
>    CFGBlock *Decision = createBlock(false);
>    Decision->setTerminator(CFGTerminator(Context.TerminatorExpr, true));
> -  addSuccessor(Decision, Block);
> -  addSuccessor(Decision, FalseSucc ? FalseSucc : Context.Succ);
> +  addSuccessor(Decision, Block, !ConditionVal.isFalse());
> +  addSuccessor(Decision, FalseSucc ? FalseSucc : Context.Succ,
> +               !ConditionVal.isTrue());
>    Block = Decision;
>  }
>
> @@ -3725,16 +3720,8 @@ CFGBlock *CFGBuilder::VisitConditionalOp
>    CFGBlock *ConditionBlock = Block;
>    CFGBlock *ConditionSucc = Succ;
>    TryResult ConditionVal = tryEvaluateBool(E->getCond());
> -
> -  if (ConditionVal.isKnown()) {
> -    if (ConditionVal.isTrue()) {
> -      VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary, Context);
> -    } else {
> -      assert(ConditionVal.isFalse());
> -      VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary, Context);
> -    }
> -    return Block;
> -  }
> +  TryResult NegatedVal = ConditionVal;
> +  if (NegatedVal.isKnown()) NegatedVal.negate();
>
>    TempDtorContext TrueContext(/*IsConditional=*/true);
>    VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary, TrueContext);
> @@ -3746,12 +3733,12 @@ CFGBlock *CFGBuilder::VisitConditionalOp
>    VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary, FalseContext);
>
>    if (TrueContext.TerminatorExpr && FalseContext.TerminatorExpr) {
> -    InsertTempDtorDecisionBlock(FalseContext, TrueBlock);
> +    InsertTempDtorDecisionBlock(FalseContext, NegatedVal, TrueBlock);
>    } else if (TrueContext.TerminatorExpr) {
>      Block = TrueBlock;
> -    InsertTempDtorDecisionBlock(TrueContext);
> +    InsertTempDtorDecisionBlock(TrueContext, ConditionVal);
>    } else {
> -    InsertTempDtorDecisionBlock(FalseContext);
> +    InsertTempDtorDecisionBlock(FalseContext, NegatedVal);
>    }
>    return Block;
>  }
>
> Modified: cfe/trunk/test/SemaCXX/return-noreturn.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return-noreturn.cpp?rev=215128&r1=215127&r2=215128&view=diff
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/return-noreturn.cpp (original)
> +++ cfe/trunk/test/SemaCXX/return-noreturn.cpp Thu Aug  7 13:44:19 2014
> @@ -185,11 +185,11 @@ int testTernaryConditionalNoreturnFalseB
>
>  int testConditionallyExecutedComplexTernaryTrueBranch(bool value) {
>    value || (true ? NoReturn() : true);
> -} // expected-warning {{control may reach end of non-void function}}
> +}

I'm a bit confused as to why /this/ warning would go away when you add
the optimstic/pessimistic edges. This warning should still fire, as,
when 'value' is false, control does reach the end of a non-void
function.

The warnings I would expect to not fire with the kind of change Ted
was suggesting (and I don't know if this is a pre-existing problem, an
issue with this patch, or an issue with my understanding) would be
things like -Wunreachable-code, probably in cases like:

true ? NoReturn() : true;
func(); // -Wunreachable-code should fire here

ARCH_CONST ? NoReturn() : true;
func(); // don't warn that this is unreachable, because ARCH_CONST
could vary by build

Or something like that - I forget which particular heuristics Ted put
in to suppress build-varying constants.

>  int testConditionallyExecutedComplexTernaryFalseBranch(bool value) {
>    value || (false ? true : NoReturn());
> -} // expected-warning {{control may reach end of non-void function}}
> +}
>
>  int testStaticallyExecutedLogicalOrBranch() {
>    false || NoReturn();
> @@ -209,7 +209,7 @@ int testStaticallySkppedLogicalAndBranch
>
>  int testConditionallyExecutedComplexLogicalBranch(bool value) {
>    value || (true && NoReturn());
> -} // expected-warning {{control may reach end of non-void function}}
> +}
>
>  #if __cplusplus >= 201103L
>  namespace LambdaVsTemporaryDtor {
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits