Paths
| Differential D85984
[analyzer] Add a new checker alpha.cplusplus.CPlusPlus11Lock Needs ReviewPublic Authored by ASDenysPetrov on Aug 14 2020, 11:22 AM.
Details
Summary This checker is made above PthreadLockChecker and works the same. It is adapted for the next primitives:
Base checker class is extended for shared semantics checks. P.S. Test file is quite big. But it just consists of the same blocks for each mutex kind. Except shared semantics has a bit different tests.
Diff Detail Event TimelineHerald added subscribers: cfe-commits, steakhal, martong and 9 others. · View Herald TranscriptAug 14 2020, 11:22 AM Comment Actions No-no, recursive locks are much more complicated than that, please do them separately. You'll have to discriminate between a real double lock / double unlock bug and an actual valid use of the recursive mutex in order to emit any warnings at all. That's completely different checker logic. Comment Actions
I think, I got the message. The real recursive logic can be caught here: std::recursive_mutex rm;
void recur1() {
recur2();
}
void recur2() {
rm.lock();
recur1(); // here we can ignore twice lock, can't we?
}Comment Actions Added support of:
ASDenysPetrov added a parent revision: D87146: [analyzer] Implement shared semantics checks for XNU functions in PthreadLockChecker. Comment Actions I have checked only your test, but the readability of the reports should be improved. Such as: void stms_bad2() { stm1.lock(); // expected-note {{Previously locked here}} stm1.lock_shared(); // expected-warning {{This lock has already been acquired}} } void stm_bad3() { stm1.lock(); // hmm, might be a good idea to put one note here too stm2.lock(); // expected-note {{Previously locked mutex}} stm1.unlock(); // expected-warning {{This was not the most recently acquired lock. Possible lock order reversal}} stm2.unlock(); // no-warning }
Comment Actions
NoQ added inline comments.
This revision now requires changes to proceed.Sep 21 2020, 10:38 PM Comment Actions
Comment Actions @NoQ , considered your suggestions about the case of recursive mutexes. Implemented this feature. Now this works like below: recursive_mutex rm; rm.lock(); rm.lock(); // no-warning recursive_timed_mutex rtm; rtm.lock(); rtm.lock(); // no-warning Please look.
Revision Contents
Diff 289190 clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
clang/test/Analysis/Checkers/CPlusPlus11LockChecker.cpp
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I repeat, this is a false positive. Recursive mutexes can be locked twice, that's the whole point.