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