[clang] [clang][analyzer] Introduce modeling for threading related checkers (PR #109636)

[Balazs Benics via cfe-commits]

Endre =?utf-8?q?Fülöp?= <endre.fulop at sigmatechnology.com>,
Endre =?utf-8?q?Fülöp?= <endre.fulop at sigmatechnology.com>,
Endre =?utf-8?q?Fülöp?= <endre.fulop at sigmatechnology.com>,
Endre =?utf-8?q?Fülöp?= <endre.fulop at sigmatechnology.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/109636 at github.com>


https://github.com/steakhal requested changes to this pull request.

I'm sharing my unfinished review as I have it now to leave you a feedback.
In the LLVM community we encourage contributors to [develop incrementally](https://llvm.org/docs/DeveloperPolicy.html#incremental-development), and post chunks in a reviewable sizes.
Landing a completely new modeling in 1 PR and and then drooping all the old implementations in another PR is not something that fits really well this model.

This would affect multiple checkers, some of which are in heavy use at places where continuity is a key asset. I don't see a way to review these two changes with confidence.
Review time usually grows exponentially with the size of the PR if done right.

There are other ways to gain confidence, for example, by having a huge refactor (and by nature an NFC change) that we can verify at scale; and then start the increments from there, changing the desired behavior. This may need to have tricks in the "newer", refactored implementation to leave room for keeping the bugs we had in the original implementation, what you will later fix. The important part is that they are not done in the same PR.

To put this into perspective, in the Clang Static Analyzer since 2019.10.25 (`llvmorg-10-init`), in apprx. 5 years, we had only 15 commits that were larger than 500 lines that don't have NFC in their title - only counting changes in `clang/lib/StaticAnalyzer` and `clang/include/clang/StaticAnalyzer`.
```
hash (ins+del) title
0202c3596c52d453d1e9e5a43d7533b83444df4e 867 - [analyzer] CastValueChecker: Store the dynamic types and casts
82923c71efa57600d015dbc281202941d3d64dde 518 - [analyzer] Add Fuchsia Handle checker
9a08a3fab9993f9b93167de5c783dfed6dd7efc0 2339 - [Analyzer] Split container modeling from iterator modeling
f5086b3803ac2f908a734bbb2c7a50018fb3cd8c 941 - [analyzer] StdLibraryFunctionsChecker refactor: remove macros
239c53b72b18d6fd6c5ad9a6d27cd09b950dc97a 709 - [analyzer] Track runtime types represented by Obj-C Class objects
f7c7e8a523f56b0ed1b14c0756ba4e5d1ccb48d2 511 - [Analyzer][WebKit] RefCntblBaseVirtualDtorChecker
db4d5f7048a26a7708821e46095742aecfd8ba46 614 - [analyzer][StdLibraryFunctionsChecker] Add POSIX file handling functions
b13d9878b8dcef4354ddfc86f382ca9b537e65aa 696 - [analyzer][solver] Track symbol equivalence
a787a4ed16d6867f56d81159a8fcf2b711d18a8a 1228 - [analyzer][StdLibraryFunctionsChecker] Use Optionals throughout the summary API
11d2e63ab0060c656398afd8ea26760031a9fb96 849 - [analyzer][StdLibraryFunctionsChecker] Separate the signature from the summaries
170c67d5b8cc58dd8a4bd0ea7c5ca02290fac39c 604 - [analyzer] Use the MacroExpansionContext for macro expansions in plists
02b51e5316cd501ede547d0223e0f5416ebd9845 789 - [analyzer][solver] Redesign constraint ranges data structure
957014da2d2791359181d89a04a0d27da65474d4 555 - [clang][Analyzer] Add errno state to standard functions modeling.
343bdb10940cb2387c0b9bd3caccee7bb56c937b 565 - [analyzer] Show taint origin and propagation correctly
527fcb8e5d6b1d491b6699cde818db1127bbb12c 521 - [analyzer] Add std::variant checker (#66481)
```
Out of this list, I can only see 3 commits where we merged a semantic change that was not a refactor/mechanical change or adds a brand new checker. 2 of which is touching the Solver, where it's unexpectedly hard to land increments. And the remaining 1 was `Show taint origin` where I regret not pushing back and we had serious bugs because of the improper quality of review - I admit. This served me a lesson, which is why I speak up now.

https://github.com/llvm/llvm-project/pull/109636