r215114 - Only have one path in the CFG for ternaries if the condition is known.

[Manuel Klimek]

On Thu, Aug 7, 2014 at 7:06 PM, Ted Kremenek <kremenek at apple.com> wrote:

> Hi Manuel,
>
> This is a good optimization, but I worry about its implications on
> -Wunreachable-code.  We have a whole bunch of heuristics in
> -Wunreachable-code to not warn about dead code that is really only
> conditionally dead because of platform-specific constants.  For example:
>
>   if (sizeof(foo) > 4) { /* possibly dead; -Wunreachable-code doesn't warn
> */ )
>
> To model this in the CFG, we actually still add the successors, but mark
> them as being possibly unreachable.  For example, here is some code
> elsewhere in CFG.cpp:
>
>   addSuccessor(Block, ThenBlock, /* isReachable = */ !KnownVal.isFalse());
>   addSuccessor(Block, ElseBlock, /* isReachable = */ !KnownVal.isTrue());
>
> What happens is that most clients of the CFG see that the branch is dead,
> but other clients can recover the original edge.  By just outright pruning
> the edge we lose this information.  We want the same heuristics for dead
> code warnings to apply to ternary operators as well.  Instead of doing an
> early return, can you just use this modified version of addSuccessor?
>

Ok, I have a current fix for a more obvious bug and will add tests for
platform specific constants next. Otherwise also feel free to revert in the
meantime if it breaks something for you...


>
> Ted
>
>
> > On Aug 7, 2014, at 7:25 AM, Manuel Klimek <klimek at google.com> wrote:
> >
> > Author: klimek
> > Date: Thu Aug  7 09:25:43 2014
> > New Revision: 215114
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=215114&view=rev
> > Log:
> > Only have one path in the CFG for ternaries if the condition is known.
> >
> > The return type analysis requires that the CFG is simplified when the
> > truth values of branches are statically known at analysis time.
> >
> > 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=215114&r1=215113&r2=215114&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/Analysis/CFG.cpp (original)
> > +++ cfe/trunk/lib/Analysis/CFG.cpp Thu Aug  7 09:25:43 2014
> > @@ -3711,15 +3711,24 @@ CFGBlock *CFGBuilder::VisitConditionalOp
> >   VisitForTemporaryDtors(E->getCond(), false, Context);
> >   CFGBlock *ConditionBlock = Block;
> >   CFGBlock *ConditionSucc = Succ;
> > +  TryResult ConditionVal = tryEvaluateBool(E->getCond());
> >
> >   TempDtorContext TrueContext(/*IsConditional=*/true);
> > -  VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary,
> TrueContext);
> > +  if (!ConditionVal.isFalse()) {
> > +    VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary,
> TrueContext);
> > +    if (ConditionVal.isTrue())
> > +      return Block;
> > +  }
> >   CFGBlock *TrueBlock = Block;
> >
> >   Block = ConditionBlock;
> >   Succ = ConditionSucc;
> >   TempDtorContext FalseContext(/*IsConditional=*/true);
> > -  VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary,
> FalseContext);
> > +  if (!ConditionVal.isTrue()) {
> > +    VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary,
> FalseContext);
> > +    if (ConditionVal.isFalse())
> > +      return Block;
> > +  }
> >
> >   if (TrueContext.TerminatorExpr && FalseContext.TerminatorExpr) {
> >     InsertTempDtorDecisionBlock(FalseContext, TrueBlock);
> >
> > Modified: cfe/trunk/test/SemaCXX/return-noreturn.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return-noreturn.cpp?rev=215114&r1=215113&r2=215114&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/test/SemaCXX/return-noreturn.cpp (original)
> > +++ cfe/trunk/test/SemaCXX/return-noreturn.cpp Thu Aug  7 09:25:43 2014
> > @@ -159,6 +159,22 @@ int testTernaryUnconditionalNoreturn() {
> >   true ? NoReturn() : NoReturn();
> > }
> >
> > +int testTernaryStaticallyConditionalNoretrunOnTrue() {
> > +  true ? NoReturn() : Return();
> > +}
> > +
> > +int testTernaryStaticallyConditionalRetrunOnTrue() {
> > +  true ? Return() : NoReturn();
> > +} // expected-warning {{control reaches end of non-void function}}
> > +
> > +int testTernaryStaticallyConditionalNoretrunOnFalse() {
> > +  false ? Return() : NoReturn();
> > +}
> > +
> > +int testTernaryStaticallyConditionalRetrunOnFalse() {
> > +  false ? NoReturn() : Return();
> > +} // expected-warning {{control reaches end of non-void function}}
> > +
> > int testTernaryConditionalNoreturnTrueBranch(bool value) {
> >   value ? (NoReturn() || NoReturn()) : Return();
> > } // expected-warning {{control may reach end of non-void function}}
> >
> >
> > _______________________________________________
> > 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/262204cf/attachment.html>