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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
