[PATCH] D40463: [analyzer] Fix false negative on post-increment of uninitialized variable.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:1046
     if (V2_untested.isUnknownOrUndef()) {
       Bldr.generateNode(U, *I, state->BindExpr(U, LCtx, V2_untested));
+
----------------
lebedev.ri wrote:
> dcoughlin wrote:
> > Instead of generating a node node here, you'll want to generate a new state:
> > ```
> > state = state->BindExpr(U, LCtx, V2_untested);
> > ```
> > and use that state in the call to evalStore().
> > 
> > Otherwise, you'll end up exploring from both the new generated node and from *I.
> > 
> > Here is a test that demonstrates why this is a problem. You should add it to your tests.
> > 
> > ```
> > void foo() {
> >   int x;
> > 
> >   x++; // expected-warning {{The expression of the unary operator is an uninitialized value. The computed value will also be garbage}}
> > 
> >   clang_analyzer_warnIfReached(); // no-warning
> > 
> > ```
> > The undefined assignment checker generates what we call "fatal" errors. These errors are so bad that it decides not to explore further on that path to report additional errors. This means that the analyzer should treat any code that is dominated by such an error as not reachable. You'll need to add the `debug.ExprInspection` checker to your test RUN line and a prototype for clang_analyzer_warnIfReached() for this test to to work.
> > 
> > Instead of generating a node node here, you'll want to generate a new state:
> > ```
> > state = state->BindExpr(U, LCtx, V2_untested);
> > ```
> > and use that state in the call to evalStore().
> Hm, so the only change needed here is
> ```diff
> - Bldr.generateNode(U, *I, state->BindExpr(U, LCtx, V2_untested));
> + state = state->BindExpr(U, LCtx, V2_untested);
> ```
> ?
Yes, that's right. What you have here looks good to me.


================
Comment at: test/Analysis/malloc-plist.c:13
         int *p = malloc(12);
-        (*p)++;
+        (*p)++; // expected-warning {{The expression of the unary operator is an uninitialized value. The computed value will also be garbage}}
+        *p = 0; // no-warning
----------------
lebedev.ri wrote:
> dcoughlin wrote:
> > Once you change the core modeling to not generate an extra node, you'll want to initialize `*p` to `0` or something to preserve the intent of this test. For this test it is important that the increment not stop further exploration of the path.
> Hmm, *this* test did not break after i changed `ExprEngine::VisitIncrementDecrementOperator()`.
> Did i change it wrong?
Aah, my mistake. I though this test was passing -verify (which verifies expected-warning{{}} contents). Instead, it is only checking the plists, so the missing expected leak would only be caught by the plist change.

I suggest changing the test to:
```
...
    if (in > 5) {
        int *p = malloc(12);
        *p = 0;
        (*p)++;
    }
...
```

So that the test doesn't generate a fatal error for the access to uninitialized memory and instead continues to check for the path of the leak.


================
Comment at: test/Analysis/malloc-plist.c:112
+    (*p)++; // expected-warning {{The expression is an uninitialized value. The computed value will also be garbage}}
+    *p = 0; // no-warning
+    (*p)++; // no-warning
----------------
Here you should stick the `*p = 0;` before the post increment so the rest of the code is exercised.


Repository:
  rC Clang

https://reviews.llvm.org/D40463