[cfe-commits] r160494 - /cfe/trunk/lib/Analysis/UninitializedValues.cpp

[Ted Kremenek]

Sorry to be late to the party guys.  We should just fix this in the analyzer.  I'll take a look.

On Jul 24, 2012, at 6:12 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Tue, Jul 24, 2012 at 5:54 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> On Jul 24, 2012, at 5:25 PM, Jordan Rose wrote:
> > On Jul 24, 2012, at 2:04 PM, Richard Smith wrote:
> >> On Fri, Jul 20, 2012 at 4:03 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> >> On Wed, Jul 18, 2012 at 9:59 PM, Ted Kremenek <kremenek at apple.com> wrote:
> >> Author: kremenek
> >> Date: Wed Jul 18 23:59:05 2012
> >> New Revision: 160494
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=160494&view=rev
> >> Log:
> >> Simplify UninitializedValues.cpp by removing logic to handle the previous (imprecise) representation
> >> of '&&' and '||' in the CFG.  This is no longer needed
> >>
> >> Sadly, that appears to be untrue. We now produce a bogus -Wuninitialized warning on this:
> >>
> >> int x(int*); int f(bool b) { int n = (b || x(&n)) ? 0 : n; return n; }
> >>
> >> More generally, && and || as the LHS of a ?: still produce a CFG with false edges.
> >>
> >> Fixed in r160691.
> >
> > This broke one of our internal buildbots for the static analyzer, on precisely this case (|| and ?:) in ctype.h. Here's a simplified test case:
> >
> > // clang -cc1 -analyze -analyzer-checker=core -x c
> > int isctype(char c, unsigned long f)
> > {
> >        return (c < 1 || c > 10) ? 0 : !!(c & f);
> > }
> >
> > This is the assertion:
> >
> >> Assertion failed: (X.isUndef()), function VisitGuardedExpr, file ExprEngineC.cpp, line 597.
> >
> > I don't remember /why/ we have to pass the decision Expr through an UndefinedVal here, but we're clearly confused by this. I think we're still expecting to see the || in the CFG before the ?:, and with this change that doesn't seem to be the case anymore.
> 
> More info: when the analyzer sees a ||, it walks back through the path to find out if we took the true path or the false path, and then uses that to decide which expression to use as the branch value. ?: expects to get that answer from its condition, but we aren't evaluating the || anymore.
> 
> I'm not sure what's the right thing to do. This CFG is /mostly/ equivalent to the previous ones, and it is more efficient...but it means that clients have to reason about branches with short-circuit conditions specially. It's just not an issue in the analyzer for 'if' and friends because those are top-level statements -- they don't need to be visited for a value as well as a branch.
> 
> Ted probably has some guidance here.
> 
> I don't know enough about the static analyzer to formulate a fix here. If this is causing you an immediate problem, please feel free to revert both r160691 and r160494 (Ted's change caused a -Werror build break for us).

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120725/a7ce96ff/attachment.html>