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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp:64
+    if (const UnaryOperator *U = dyn_cast<UnaryOperator>(StoreE)) {
+      str = "The expression of the unary operator is an uninitialized value. "
+            "The computed value will also be garbage";
----------------
dcoughlin wrote:
> "Unary operator" is probably not something we should expect users to know about. How about just "The expression is an uninitialized value. The computed value will also be garbage."
> 
Yep, i did not like my wording either :)


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:1051
+      ExplodedNodeSet Dst3;
+      evalStore(Dst3, U, U, *I, state, loc, V2_untested);
+      Bldr.addNodes(Dst3);
----------------
xazax.hun wrote:
> This will trigger checkBind when unknown value is pre/post incremented. I wonder if this is the desired behavior. Couldn't you achieve the same on the checker side by having a checkPreStmt hook?
> 
> @NoQ, @dcoughlin what do you think about triggering checkBind here? In fact, there is a bind during pre/post increment/decrement. But do we want these events to be visible for checkers with undefined values?
> This will trigger checkBind when unknown value is pre/post incremented. I wonder if this is the desired behavior. Couldn't you achieve the same on the checker side by having a checkPreStmt hook?
The current approach is what was suggested in the https://bugs.llvm.org/show_bug.cgi?id=35419#c6, unless i have misread it of course.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:1046
     if (V2_untested.isUnknownOrUndef()) {
       Bldr.generateNode(U, *I, state->BindExpr(U, LCtx, V2_untested));
+
----------------
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);
```
?


================
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
----------------
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?


================
Comment at: test/Analysis/malloc-plist.c:111
     p = (int*)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
----------------
dcoughlin wrote:
> Same point applies here.
Same here.


Repository:
  rC Clang

https://reviews.llvm.org/D40463