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

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

Thanks for tackling this! There is one major comment inline (you are generating an extra node that you shouldn't, which is causing the analyzer to explore code it shouldn't) and a suggestion for simpler diagnostic text.



================
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";
----------------
"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."



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



================
Comment at: test/Analysis/malloc-plist.c:2
 // RUN: rm -f %t
 // RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,unix.Malloc -analyzer-output=plist -analyzer-config path-diagnostics-alternate=false -o %t %s
 // RUN: FileCheck -input-file %t %s
----------------
xazax.hun wrote:
> In case you willing to replace the plist based test with `-analyzer-output=text` based, it would be awesome, though not required for this patch to be accepted.
Let's hold off on replacing the plist test with text. We'll need to make sure we're still testing plist emission separately.


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


================
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
----------------
Same point applies here.


================
Comment at: test/Analysis/objc-for.m:131
     int i;
     int j;
     for (NSString *a in A) {
----------------
You should just initialize `j` in order to preserve the test's intent. (And similarly for the other new warnings in this file.


================
Comment at: test/Analysis/uninit-const.c:127
+  int x; // expected-note 5 {{'x' declared without an initial value}}
+  x++; // expected-warning {{The expression of the unary operator is an uninitialized value. The computed value will also be garbage}}
+       // expected-note at -1 {{The expression of the unary operator is an uninitialized value. The computed value will also be garbage}}
----------------
You'll need to split this test into multiple functions once you stop generating the extra node.


================
Comment at: test/Analysis/uninit-const.cpp:11
 
+// https://bugs.llvm.org/show_bug.cgi?id=35419
+void f11(void) {
----------------
I don't think this test is needed. The logic for C vs. C++ is shared for your new functionality, so I think it is sufficient to just test the C case.


Repository:
  rL LLVM

https://reviews.llvm.org/D40463