[PATCH] D64232: [analyzer] Prune calls to functions with linear CFGs that return a non-zero constrained value

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1052
+        // to return that every time.
+        if (N->getCFG().isLinear())
+          WouldEventBeMeaningless = true;
----------------
This time i'd rather go for "has exactly 3 blocks", because we're mostly interested in the CFG being "syntactically linear" rather than "semantically linear". I.e., if the CFG has an if-statement with a compile-time-constant condition, i'd rather show the path, because who knows what does the programmer think about this condition.


================
Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:185
     return true;
   return coin(); // tracking-note{{Returning value}}
 }
----------------
Szelethus wrote:
> This note is meaningful, the bug would not have occurred if `coin()` wasn't assumed to be false.
Mmmmm. Mmmmm. Dunno, this is getting complicated very quickly. Say, if you replace `return true` with `return false` on the previous line, the note becomes meaningless again. I don't see a direct connection between meaningfulness and linearness.

The note in this example is relatively meaningful indeed, but i'm not sure it's so much meaningful that it's worth the noise. It's still not surprising for me that `flipCoin()` occasionally returns false. I do believe that it may be sometimes surprising that `flipCoin()` may return false on the *current path*, which would make the note meaningful. However, note that we don't prove that it may return false, we only push the assumption one step deeper, i.e. put the blame on `coin()` instead of `flipCoin()`, but we still fully trust our assumption about `coin()` which may have the same problem. And if we had an actual proof that it may return false, we would have had a concrete-false return value, so the patch wouldn't apply.

I guess some experimental data would be great to have. 


================
Comment at: clang/test/Analysis/uninit-vals.c:181
 void testUseHalfPoint() {
-  struct Point p = getHalfPoint(); // expected-note{{Calling 'getHalfPoint'}}
-                                   // expected-note at -1{{Returning from 'getHalfPoint'}}
-                                   // expected-note at -2{{'p' initialized here}}
+  struct Point p = getHalfPoint(); // expected-note{{'p' initialized here}}
   use(p); // expected-warning{{uninitialized}}
----------------
Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Huh, so there's not even a note in `getHalfPoint()`, just calling..returning? This definitely needs some attention from `NoStoreFuncVisitor`.
> > > 
> > > Generally, i think this is probably the single place where we do really want some info about what happens in `getHalfPoint()`. The report that consists only of "p is initialized..." and "...p is uninitialized" is pretty weird. Btw, could you write down the full warning text in this test? How bad this actually is?
> > Wild idea: `UninitializedObjectChecker` exposes it's main logic through a header file, how about we use it when we find a read of an uninitialized region?
> Passed-by-value struct argument contains uninitialized data (e.g., field: 'y')
> 
> Quite good imo.
What specific logic are you talking about? You mean it'd scan the structure for uninitialized fields and present the results of the scan to the user in a note?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64232/new/

https://reviews.llvm.org/D64232