Please run this on open source projects and upload the results.

LGTM

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.

A high level comment before getting into (even more) nitty gritty stuff. But shut me down if I misunderstood whats happening.

In D144269#4143066, @NoQ wrote:

The challenging part with note tags is how do you figure out whether your bug report is taint-related. The traditional solution is to check the BugType but in this case an indeterminate amount of checkers may emit taint-related reports.

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?

LGTM!

A small nit, otherwise LGTM.

Awesome, been a long time coming!!

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.

In D142354#4078450, @NoQ wrote:

Interesting, what specific goals do you have here? Are you planning to find specific bugs (eg. force-unwrap to a wrong type) or just to model the semantics?

LGTM! I think I prefer this solution anyways. Please commit (the entire patchstack).

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).

Mostly LGTM, but I see that you have tests for the predecessor patch here as well, so I'll accept both at once.

In D140387#4021806, @Szelethus wrote:

Would be possible to test the errno specific changes as well?

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?

In D140387#4007751, @balazske wrote:

This patch and D140395 is (almost) the same code as D135360 and D135247. The changes are separated for the different checkers. Tests are added at the second patch.

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?

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...

In D137790#3992216, @balazske wrote:

On the postgres results, the second is one that can be fixed in the checker (add special cases to StdLibraryFunctionsChecker for zero len or size fread and fwrite arguments). The others are false positives because the error path is impossible because implicit constraints (what is not known to the analyzer) on variables.

In D137790#3992216, @balazske wrote:

Some reports can be found here (if the link works and the data does not expire), the runs stored on 2022-12-09.

Results appeared only projects "postgres" and "curl" (from memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres,libwebm) from checkers StdCLibraryFunctionArgs and Errno.

On the postgres results, the second is one that can be fixed in the checker (add special cases to StdLibraryFunctionsChecker for zero len or size fread and fwrite arguments). The others are false positives because the error path is impossible because implicit constraints (what is not known to the analyzer) on variables.

These curl results look faulty, the last fclose call looks not recognized.

In D135360#3981420, @balazske wrote:

My current approach is that the POSIX is more strict than the C standard (POSIX allows a subset of what C allows). I do not see (errno related) contradiction between these standards

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?

The patch looks OK now, I'll get to inspecting the others.

In D135247#3977403, @balazske wrote:

The "strange" test failures that showed up earlier were probably caused by a bug that is fixed in the D137722. I just read that this patch is rebased to D137722 too, fixed the dependency stack.

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.

In D135360#3862260, @martong wrote:

In D135360#3839890, @balazske wrote:

I found some anomalies during development:

• If the checker StdCLibraryFunctions is added as dependency for alpha.unix.Stream in checkers.td I get some "unexplainable" test failures.

Could you please elaborate? I don't see how to help you with it without seeing more details.

Sorry abour my previous reply, I messed up the thread I was replying to. I better see what is going on.

Seems like the issues mentioned above are real, but orthogonal to this patch. Would it be okay to address them in followup patches? @martong @NoQ

Some early results:

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.

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?

I suppose enough has been said! Well done! Please attend to the rest of the inlines and commit at your convenience.

LLVM finished as well, with 2 new warnings! The link above is the complete evaluation of your patch on our benchmark.

Some early results: https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=qtbase_v6.2.0_pod_array_bindings_baseline&run=protobuf_v3.13.0_pod_array_bindings_baseline&items-per-page=100&sort-by=runDate&sort-desc=true&newcheck=qtbase_v6.2.0_pod_array_bindings&newcheck=protobuf_v3.13.0_pod_array_bindings&is-unique=on&diff-type=New

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.

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.

No need for post commit fixes, just general observations since I noticed them.

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.

LGTM

Fixes according to reviewer comments.

Very well :) Let's abandon this in its current state, I share this sentiment:

LGTM! You did check whether a missing doc field will actually trigger this error, right?

LGTM on my end, this is awesome!

Seems like all new files are missing the header blurb about the licence.

Nice!

In D120992#3364804, @NoQ wrote:

This check checks must-properties/all-paths properties. This has to be a data flow / CFG-based warning. I don't think there's a way around.

Remove a newline.

Can we reopen this if the code is not upstream at this time?

Sorry for the slack, I assumed this was accepted already. Thanks!

Fixes according to reviewer comments.

• Rename from .*Imprecise to .*AsWritten.
• Copy comments to relevant functions.

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!

Sounds about right! Just a nit, otherwise LGTM.

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).

In D119004#3297025, @steakhal wrote:

I strongly belive that this should be an overload to the existing 'matches' API. Maybe add a comment that prefer the other overload if can. But having an overload for that alread implies this anyway.

Move CallDescription specific changes to D119004.

Ping ^-^

Fix tests, mention that this is purely a heuristic.