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@-1{{Returning from
'getHalfPoint'}}
- // expected-note@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
