This is an archive of the discontinued LLVM Phabricator instance.

[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:

  • std::mutex
  • std::timed_mutex
  • std::recursive_mutex
  • std::recursive_timed_mutex
  • std::shared_mutex
  • std::shared_timed_mutex

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 Timeline

ASDenysPetrov created this revision.Aug 14 2020, 11:22 AM
ASDenysPetrov requested review of this revision.Aug 14 2020, 11:22 AM

Added recursive_mutex support.

ASDenysPetrov edited the summary of this revision. (Show Details)Aug 25 2020, 10:04 AM
ASDenysPetrov edited the summary of this revision. (Show Details)
NoQ added a comment.Aug 25 2020, 1:45 PM

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.

ASDenysPetrov added a comment.EditedAug 26 2020, 12:23 AM

@NoQ

That's completely different checker logic.

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? 
}
ASDenysPetrov edited the summary of this revision. (Show Details)

Added support of:

  • std::timed_mutex
  • std::recursive_timed_mutex
  • std::shared_mutex
  • std::shared_timed_mutex

Added shared semantics checks and correspondent tests.

ASDenysPetrov edited the summary of this revision. (Show Details)Aug 31 2020, 6:05 AM
ASDenysPetrov edited the summary of this revision. (Show Details)Aug 31 2020, 6:08 AM

Added timed functions support and tests for them.

I have checked only your test, but the readability of the reports should be improved.
You frequently refer to previous events, such as This lock has already been unlocked, This lock has already been acquired, etc.
It isn't clear to the reader where do you refer to. IMO you should put a NoteTag at the interesting locations to achieve more readable diagnostics.

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
}
NoQ added inline comments.Sep 8 2020, 1:50 AM
clang/test/Analysis/Checkers/CPlusPlus11LockChecker.cpp
379–382

I repeat, this is a false positive. Recursive mutexes can be locked twice, that's the whole point.

@steakhal

the readability of the reports should be improved.

Absolutely agree. Let's do this in the next patches :)

@NoQ
You've recommended to extend PthreadLockChecker with STL mutexes. I think I've done. Could you make a quick look, please?

clang/test/Analysis/Checkers/CPlusPlus11LockChecker.cpp
379–382

Yes, I remember. I think you've mixed up with my previous attempts of introducting a new checks for recursive mutexes.
This is not this case. This kind of checks already exists in PthreadLockChecker before.
I've just reused this check for all kind of STL mutexes.

If you look at void bad1() in clang\test\Analysis\pthreadlock.c you can find the same case.

NoQ requested changes to this revision.Sep 21 2020, 10:38 PM
NoQ added inline comments.
clang/test/Analysis/Checkers/CPlusPlus11LockChecker.cpp
379–382

I've just reused this check for all kind of STL mutexes.

You're taking code for mushing potatoes and applying it to differential calculus.

Different kinds of mutexes behave differently. This is not the same case. That test is a true positive because that mutex is non-recursive, your test is a false positive because your mutex is recursive. In that test the program immediately hangs and there's no valid way to unhang it, in your test the programmer simply has to unlock the mutex twice and they're good to go.

This revision now requires changes to proceed.Sep 21 2020, 10:38 PM
clang/test/Analysis/Checkers/CPlusPlus11LockChecker.cpp
379–382

@NoQ, sorry. This might be a simple misunderstanging. Previously there was a question in the summary of what if we will consider a twice lock for recursive mutex as a correct case:

recursive_mutex rm;
m.lock();
m.lock(); // OK

You've answered that recursive locks are much more complicated than that (https://reviews.llvm.org/D85984#2237242) and offered to make this separately. So I decided not to make any changes and handle recursive_mutexes as all the rest. But if you suggest to do this in a single patch, so I will do.

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

A gentle ping :-)