User Details
- User Since
- Jul 19 2017, 6:59 AM (297 w, 3 d)
Tue, Mar 28
Please run this on open source projects and upload the results.
Mon, Mar 13
Thu, Mar 9
LGTM
Wed, Mar 8
We worked on this together, so I waited a bit for others to have a say in this, but this design seems like a no brainer to me. Please fix those comments, otherwise LGTM.
Mar 2 2023
Feb 27 2023
A high level comment before getting into (even more) nitty gritty stuff. But shut me down if I misunderstood whats happening.
Feb 23 2023
Feb 22 2023
Ugh, I admit, its a little hard to follow what happened here. You moved a lot of code around (I agree with that!), but also changed code as well. Can you just summarize what is NOT just moved code and needs a more thorough look?
Feb 14 2023
LGTM!
Feb 10 2023
A small nit, otherwise LGTM.
Feb 3 2023
Awesome, been a long time coming!!
Feb 2 2023
While we were there, we also dug into std::any, and learned that the analyzer can model it shockingly well. Hopefully we can submit a few patches that demonstrates it in a form of some test files.
Jan 25 2023
Jan 5 2023
LGTM! I think I prefer this solution anyways. Please commit (the entire patchstack).
Jan 4 2023
Please rerun the evaluation before commiting to confirm the results haven't changed! Otherwise, LGTM.
LGTM
LGTM, granted you add that test in the followup commit. If possible, I'd prefer to have features tested in the patch that added them (but this is fine for now).
Jan 3 2023
Mostly LGTM, but I see that you have tests for the predecessor patch here as well, so I'll accept both at once.
Jan 2 2023
Are you sure that the refactoring made no changes to the results? Could you maybe just run a nightly or something like that to confirm?
Does this patch fix any false positives from before, or is this just all new stuff? I ask, because I wonder whats the shortest path towards popping these checkers out of alpha, and fix what we already have. By no means am I saying that we should postpone landing this, but take a more directed attempt at tying off loose ends after this stack.
Would be possible to test the errno specific changes as well?
Dec 20 2022
Some of the changes are also present in D135247. I suppose you're in the middle of splitting those patches apart and remaking the patch stack?
Dec 19 2022
I have a fear that we may have too few results as well -- maybe we should expand our testing infrastructure with more POSIX API heavy codebases. Looking at a few new projects, https://github.com/audacity/audacity looks like a good candidate, but of course it often turns out that adding a new project to the benchmark is more troublesome than it appears...
Dec 13 2022
IIRC we talked about it would only really make sense to evaluate this patch stack as a whole, not piece by piece, but I'm not seeing results on open source projects here either. Can you please post them?
Dec 11 2022
The patch looks OK now, I'll get to inspecting the others.
Very well!
There was another problem with circular dependencies (because StdCLibraryFunctionArgsChecker had a dependency to StreamChecker, this is removed in the last patch). The checker option must be not a problem, the checker (StdLibraryFunctionsChecker) can be disabled (but is normally not because it is an apiModeling checker) or the ModelPOSIX option turned off independently if StreamChecker is enabled or not.
Okay, so the checker behaves OK if StdLibraryFunctionsChecker is disabled. As long as it doesn't crash, this is fine, you shouldn't disable it in practice anyways!
The goal (should work at the end of this patch stack) is that StreamChecker can report all bugs that it can find, and there is no case when both checkers report a bug (in different way). If ModelPOSIX is turned off and StreamChecker is enabled, for fseek for example no bug is found if stream is NULL, and value of errno is just invalidated in all cases (like it works if StreamChecker is disabled too), but the stream state and file position is still checked by StreamChecker for all functions.
This sounds reasonable. It means that no parts of StdLibraryFunctionsChecker (including its option) is a "hard" dependency.
Dec 8 2022
Dec 6 2022
Sorry abour my previous reply, I messed up the thread I was replying to. I better see what is going on.
Dec 5 2022
Oct 26 2022
Oct 25 2022
Oct 11 2022
Some early results:
Oct 7 2022
Oct 6 2022
Just a note on the test files -- I've diverged from the usual stance of just changing what the new output is, to modifying the test files. The reason is that reading an undefined value is a fatal error, leading to the analyzer to stop analyzing prematurely, and I think these cases were trying to test something else, not uninitialized value usage.
Jul 19 2022
Yeah, I'm afraid no fun is allowed on this block. On another note, kaboom is interesting, shouldn't we assume all functions to be kaboom unless proven to be woot?
Jul 13 2022
I suppose enough has been said! Well done! Please attend to the rest of the inlines and commit at your convenience.
Jul 12 2022
LLVM finished as well, with 2 new warnings! The link above is the complete evaluation of your patch on our benchmark.
Jul 7 2022
So sorry, I know I took my sweet time -- the patch looks great, and currently I'm running some analysis with it. As soon as I have the results on my hand I'd be happy to share and accept. I added a couple nots here and there, none of them are particularly interesting, feel free to attend to them when we are sure that no meaningful changes are needed to be made.
Jul 1 2022
I read https://en.cppreference.com/w/cpp/language/structured_binding carefully, and there are a number of interesting rules that might deserve their own test case, even if this isn't the patch where you solve that issue, or believe that the solution handles it without the need for special case handling.
Jun 30 2022
No need for post commit fixes, just general observations since I noticed them.
Jun 28 2022
I tried poking this from a few directions, like nasty GNU extension types, ObjCObjectPointerType, but those seem orthogonal to this patch. Looks great! I'd wait for someone else's approval as well, as I try my best to pick up the thread.
Jun 6 2022
LGTM
Apr 8 2022
Mar 30 2022
Fixes according to reviewer comments.
Very well :) Let's abandon this in its current state, I share this sentiment:
Mar 24 2022
LGTM! You did check whether a missing doc field will actually trigger this error, right?
Mar 23 2022
LGTM on my end, this is awesome!
Mar 22 2022
Mar 16 2022
Seems like all new files are missing the header blurb about the licence.
Mar 11 2022
Nice!
Mar 8 2022
Mar 6 2022
Mar 4 2022
Mar 3 2022
Mar 1 2022
Feb 25 2022
Feb 23 2022
Remove a newline.
Feb 22 2022
Can we reopen this if the code is not upstream at this time?
Feb 10 2022
Sorry for the slack, I assumed this was accepted already. Thanks!
Feb 9 2022
Fixes according to reviewer comments.
- Rename from .*Imprecise to .*AsWritten.
- Copy comments to relevant functions.
Feb 8 2022
Cheers!
LGTM! Unrelated to this review, I don't think the term 'sink' is good in a warning message, are users expected to know what that is?
LGTM!
Feb 7 2022
Sounds about right! Just a nit, otherwise LGTM.
Feb 5 2022
Now that I remember, the ever so slightly different overloads of ProgramState::getSVal is a prime example I think. I always percieved that I have the means to invoke several of them at any point, but I never really knew which one. Though, to be fair, they were not documented particularly well (at least as I remember it).
Feb 4 2022
Move CallDescription specific changes to D119004.
Feb 3 2022
Feb 1 2022
Ping ^-^
Jan 24 2022
Fix tests, mention that this is purely a heuristic.