r215128 - Mark successors as reachable/unreachable instead of changing the CFG.
Manuel Klimek
klimek at google.com
Thu Aug 7 12:49:15 PDT 2014
On Thu, Aug 7, 2014 at 9:16 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 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.
>
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.
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140807/db1dc678/attachment.html>
More information about the cfe-commits
mailing list