[PATCH] D85431: [analyzer] Implement a new checker ThreadPrimitivesChecker

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

It is an interesting checker, but it seems that this kind of checker is extremely hard in single TU/top-down analysis.  It feels like it's going to produce hell a lot of false positives in the wild.
Also `mutex` is usually a global variable - the nemesis of any static analysis tool.

In general, it would be great to have checkers for multi-threaded programs.  However, it seems like we would need some sort of conventions that we enforce.  For example, every function containing a `lock` for a mutex should also have the `unlock`.  And the checker would verify that the user follows the convention rather than has correct code.  The convention, of course, should be designed so it implies correctness.  This is the model we have with `RetainCountChecker`.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:36
+private:
+  mutable std::unique_ptr<BugType> BT;
+  CallDescription LockFunc, UnlockFunc;
----------------
These days we simply do `BugType` without `unique_ptr`s


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:46
+
+REGISTER_SET_WITH_PROGRAMSTATE(LockedMutexes, SVal)
+
----------------
You should also cleanup and remove dead symbols from the set.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:46
+
+REGISTER_SET_WITH_PROGRAMSTATE(LockedMutexes, SVal)
+
----------------
vsavchenko wrote:
> You should also cleanup and remove dead symbols from the set.
Maybe `SymbolRef` is more suitable here?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:49
+ThreadPrimitivesChecker::ThreadPrimitivesChecker()
+    : LockFunc({"std","mutex","lock"}, 0, 0), UnlockFunc({"std","mutex","unlock"}, 0, 0) {}
+
----------------
Does it conform with `clang-format`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:52
+
+std::pair<bool, bool> ThreadPrimitivesChecker::FindMutexLockOrUnlock(
+    const CallEvent &Call, CheckerContext &C) const {
----------------
I think this is better expressed with `enum` because this contract implies only 3 possible values.  I prefer to bake such contracts into the types of the actual solution so that the code is more robust.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:64
+void ThreadPrimitivesChecker::checkPostCall(const CallEvent &Call,
+                                                    CheckerContext &C) const {
+  // Find mutex::lock or mutex::unlock functions.
----------------
Looks overindented


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:74
+  // We are sure about cast here, because mutex::lock/unlock met before.
+  const auto *MCall = dyn_cast<CXXMemberCall>(&Call);
+  assert(MCall);
----------------
If we are sure then why are we using `dyn_cast` instead of `cast`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ThreadPrimitivesChecker.cpp:97
+void ThreadPrimitivesChecker::reportBug(CheckerContext &C,
+                                  const char Msg[]) const {
+  ExplodedNode *ErrNode = C.generateNonFatalErrorNode();
----------------
Looks under indented


================
Comment at: clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp:27
+  m1.lock();
+  m2.unlock(); // expected-warning{{TRUE}}
+}
----------------
I think we should also add a TODO for having a warning for no `m1.unlock()` 


================
Comment at: clang/test/Analysis/Checkers/ThreadPrimitivesChecker.cpp:49
+}
\ No newline at end of file

----------------
😱😱😱


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85431/new/

https://reviews.llvm.org/D85431