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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked 2 inline comments as done.
Szelethus added inline comments.


================
Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:185
     return true;
   return coin(); // tracking-note{{Returning value}}
 }
----------------
NoQ wrote:
> 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. 
Oh yea, I see where you are going. As I understand correctly, these are two separate problems: trying to somehow argue about other execution paths and whether dropping all constraints on a symbol is a good approach.

I should have all the results by tomorrow if all goes according to plan.


================
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}}
----------------
NoQ wrote:
> 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?
>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?
Nvm, looked at the code, realized that what I said made no sense. What we are really missing here is a `trackRegionValue()` function :^)

Btw, I wasted soooo much time on figuring out that you don't get ANY notes whatsoever if you make this a cpp file rather than a c file, only the warning... Is this intended?


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

https://reviews.llvm.org/D64232