[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 accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Sry, should have approved this ages ago >.<



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

At a glance, the behavior of this code in C and C++ is ridiculously different. The way structures are returned-by-value is one of the glaring differences between C and C++. Even at runtime / ABI / calling convention level: in C it's acceptable to simply pass the structure through registers (or put it on the stack, wherever the return value normally lives), however in C++ the calling convention usually requres you to pass return address as a separate hidden parameter so that to use it as "this" in the structure's constructor (and then later use it for RVO and NRVO which may span multiple stack frames) . And the Static Analyzer also deals with completely different ASTs. So i'm not surprised that there's a difference.

But i would also probably not want there to be a difference. If you could turn it into a sensible bug report i guess it could be helpful.


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

https://reviews.llvm.org/D64232