Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -33,7 +33,9 @@ enum Kind { Destroyed, Locked, + SharedLocked, Unlocked, + SharedUnlocked, UntouchedAndPossiblyDestroyed, UnlockedAndPossiblyDestroyed } K; @@ -43,7 +45,9 @@ public: static LockState getLocked() { return LockState(Locked); } + static LockState getSharedLocked() { return LockState(SharedLocked); } static LockState getUnlocked() { return LockState(Unlocked); } + static LockState getSharedUnlocked() { return LockState(SharedUnlocked); } static LockState getDestroyed() { return LockState(Destroyed); } static LockState getUntouchedAndPossiblyDestroyed() { return LockState(UntouchedAndPossiblyDestroyed); @@ -55,7 +59,9 @@ bool operator==(const LockState &X) const { return K == X.K; } bool isLocked() const { return K == Locked; } + bool isSharedLocked() const { return K == SharedLocked; } bool isUnlocked() const { return K == Unlocked; } + bool isSharedUnlocked() const { return K == SharedUnlocked; } bool isDestroyed() const { return K == Destroyed; } bool isUntouchedAndPossiblyDestroyed() const { return K == UntouchedAndPossiblyDestroyed; @@ -99,7 +105,7 @@ {{"pthread_rwlock_wrlock", 1}, &PthreadLockChecker::AcquirePthreadLock}, {{"lck_mtx_lock", 1}, &PthreadLockChecker::AcquireXNULock}, {{"lck_rw_lock_exclusive", 1}, &PthreadLockChecker::AcquireXNULock}, - {{"lck_rw_lock_shared", 1}, &PthreadLockChecker::AcquireXNULock}, + {{"lck_rw_lock_shared", 1}, &PthreadLockChecker::AcquireXNULockShared}, // Try. {{"pthread_mutex_trylock", 1}, &PthreadLockChecker::TryPthreadLock}, @@ -107,14 +113,14 @@ {{"pthread_rwlock_trywrlock", 1}, &PthreadLockChecker::TryPthreadLock}, {{"lck_mtx_try_lock", 1}, &PthreadLockChecker::TryXNULock}, {{"lck_rw_try_lock_exclusive", 1}, &PthreadLockChecker::TryXNULock}, - {{"lck_rw_try_lock_shared", 1}, &PthreadLockChecker::TryXNULock}, + {{"lck_rw_try_lock_shared", 1}, &PthreadLockChecker::TryXNULockShared}, // Release. {{"pthread_mutex_unlock", 1}, &PthreadLockChecker::ReleaseAnyLock}, {{"pthread_rwlock_unlock", 1}, &PthreadLockChecker::ReleaseAnyLock}, {{"lck_mtx_unlock", 1}, &PthreadLockChecker::ReleaseAnyLock}, {{"lck_rw_unlock_exclusive", 1}, &PthreadLockChecker::ReleaseAnyLock}, - {{"lck_rw_unlock_shared", 1}, &PthreadLockChecker::ReleaseAnyLock}, + {{"lck_rw_unlock_shared", 1}, &PthreadLockChecker::ReleaseAnyLockShared}, {{"lck_rw_done", 1}, &PthreadLockChecker::ReleaseAnyLock}, // Destroy. @@ -183,23 +189,31 @@ CheckerKind CheckKind) const; void AcquireXNULock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const; + void AcquireXNULockShared(const CallEvent &Call, CheckerContext &C, + CheckerKind CheckKind) const; void TryPthreadLock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const; void TryXNULock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const; + void TryXNULockShared(const CallEvent &Call, CheckerContext &C, + CheckerKind CheckKind) const; void TryFuchsiaLock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const; void TryC11Lock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const; void AcquireLockAux(const CallEvent &Call, CheckerContext &C, const Expr *MtxExpr, SVal MtxVal, bool IsTryLock, - LockingSemantics Semantics, CheckerKind CheckKind) const; + bool IsShared, LockingSemantics Semantics, + CheckerKind CheckKind) const; // Release. void ReleaseAnyLock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const; + void ReleaseAnyLockShared(const CallEvent &Call, CheckerContext &C, + CheckerKind CheckKind) const; void ReleaseLockAux(const CallEvent &Call, CheckerContext &C, - const Expr *MtxExpr, SVal MtxVal, + const Expr *MtxExpr, SVal MtxVal, bool IsShared, + enum LockingSemantics Semantics, CheckerKind CheckKind) const; // Destroy. @@ -228,6 +242,7 @@ mutable std::unique_ptr BT_destroylock[CK_NumCheckKinds]; mutable std::unique_ptr BT_initlock[CK_NumCheckKinds]; mutable std::unique_ptr BT_lor[CK_NumCheckKinds]; + mutable std::unique_ptr BT_shared[CK_NumCheckKinds]; void initBugType(CheckerKind CheckKind) const { if (BT_doublelock[CheckKind]) @@ -242,6 +257,8 @@ CheckNames[CheckKind], "Init invalid lock", "Lock checker"}); BT_lor[CheckKind].reset(new BugType{CheckNames[CheckKind], "Lock order reversal", "Lock checker"}); + BT_shared[CheckKind].reset(new BugType{ + CheckNames[CheckKind], "Shared semantics misuse", "Lock checker"}); } }; } // end anonymous namespace @@ -345,46 +362,61 @@ void PthreadLockChecker::AcquirePthreadLock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const { - AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), false, + AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), false, false, PthreadSemantics, CheckKind); } void PthreadLockChecker::AcquireXNULock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const { - AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), false, + AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), false, false, + XNUSemantics, CheckKind); +} + +void PthreadLockChecker::AcquireXNULockShared(const CallEvent &Call, + CheckerContext &C, + CheckerKind CheckKind) const { + AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), false, true, XNUSemantics, CheckKind); } void PthreadLockChecker::TryPthreadLock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const { - AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, + AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, false, PthreadSemantics, CheckKind); } void PthreadLockChecker::TryXNULock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const { - AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, - PthreadSemantics, CheckKind); + AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, false, + XNUSemantics, CheckKind); +} + +void PthreadLockChecker::TryXNULockShared(const CallEvent &Call, + CheckerContext &C, + CheckerKind CheckKind) const { + AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, true, + XNUSemantics, CheckKind); } void PthreadLockChecker::TryFuchsiaLock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const { - AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, + AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, false, PthreadSemantics, CheckKind); } void PthreadLockChecker::TryC11Lock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const { - AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, + AcquireLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, false, PthreadSemantics, CheckKind); } void PthreadLockChecker::AcquireLockAux(const CallEvent &Call, CheckerContext &C, const Expr *MtxExpr, SVal MtxVal, bool IsTryLock, + bool IsShared, enum LockingSemantics Semantics, CheckerKind CheckKind) const { if (!ChecksEnabled[CheckKind]) @@ -451,19 +483,29 @@ // Record that the lock was acquired. lockSucc = lockSucc->add(lockR); - lockSucc = lockSucc->set(lockR, LockState::getLocked()); + auto State = IsShared ? LockState::getSharedLocked() : LockState::getLocked(); + lockSucc = lockSucc->set(lockR, State); C.addTransition(lockSucc); } void PthreadLockChecker::ReleaseAnyLock(const CallEvent &Call, CheckerContext &C, CheckerKind CheckKind) const { - ReleaseLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), CheckKind); + ReleaseLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), false, + NotApplicable, CheckKind); +} + +void PthreadLockChecker::ReleaseAnyLockShared(const CallEvent &Call, + CheckerContext &C, + CheckerKind CheckKind) const { + ReleaseLockAux(Call, C, Call.getArgExpr(0), Call.getArgSVal(0), true, + NotApplicable, CheckKind); } void PthreadLockChecker::ReleaseLockAux(const CallEvent &Call, CheckerContext &C, const Expr *MtxExpr, - SVal MtxVal, + SVal MtxVal, bool IsShared, + enum LockingSemantics Semantics, CheckerKind CheckKind) const { if (!ChecksEnabled[CheckKind]) return; @@ -486,6 +528,20 @@ reportBug(C, BT_destroylock, MtxExpr, CheckKind, "This lock has already been destroyed"); return; + } else if (LState->isLocked()) { + if (IsShared) { + reportBug( + C, BT_shared, MtxExpr, CheckKind, + "This lock has been acquired exclusively, but shared unlock used"); + return; + } + } else if (LState->isSharedLocked()) { + if (!IsShared) { + reportBug( + C, BT_shared, MtxExpr, CheckKind, + "This lock has been acquired as shared, but exclusive unlock used"); + return; + } } } @@ -503,7 +559,9 @@ state = state->set(LS.getTail()); } - state = state->set(lockR, LockState::getUnlocked()); + auto State = + IsShared ? LockState::getSharedUnlocked() : LockState::getUnlocked(); + state = state->set(lockR, State); C.addTransition(state); } Index: clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h =================================================================== --- clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h +++ clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h @@ -19,6 +19,7 @@ void *foo; } lck_rw_t; +typedef pthread_mutex_t lck_mtx_t; typedef pthread_mutex_t lck_mtx_t; extern void fake_system_function(); @@ -34,6 +35,8 @@ extern void lck_mtx_lock(lck_mtx_t *); extern void lck_mtx_unlock(lck_mtx_t *); extern boolean_t lck_mtx_try_lock(lck_mtx_t *); +extern boolean_t lck_rw_try_lock_shared(lck_rw_t *lck); +extern boolean_t lck_rw_try_lock_exclusive(lck_rw_t *lck); extern void lck_mtx_destroy(lck_mtx_t *lck, lck_grp_t *grp); extern void lck_rw_lock_exclusive(lck_rw_t *lck); Index: clang/test/Analysis/pthreadlock.c =================================================================== --- clang/test/Analysis/pthreadlock.c +++ clang/test/Analysis/pthreadlock.c @@ -242,6 +242,16 @@ pthread_mutex_destroy(&local_mtx); } +void ok32(void) { + if (lck_rw_try_lock_exclusive(&rw) != 0) // no-warning + lck_rw_unlock_exclusive(&rw); // no-warning +} + +void ok33(void) { + if (lck_rw_try_lock_shared(&rw) != 0) // no-warning + lck_rw_lock_shared(&rw); // no-warning +} + void bad1(void) { @@ -502,14 +512,27 @@ } void bad32(void) { - lck_rw_lock_shared(&rw); - lck_rw_unlock_exclusive(&rw); // FIXME: warn - should be shared? - lck_rw_lock_exclusive(&rw); - lck_rw_unlock_shared(&rw); // FIXME: warn - should be exclusive? -} - -void bad33(void) { pthread_mutex_lock(pmtx); fake_system_function(); pthread_mutex_lock(pmtx); // expected-warning{{This lock has already been acquired}} } + +void bad33(void) { + lck_rw_lock_exclusive(&rw); + lck_rw_unlock_shared(&rw); // expected-warning{{This lock has been acquired exclusively, but shared unlock used}} +} + +void bad34(void) { + lck_rw_lock_shared(&rw); + lck_rw_unlock_exclusive(&rw); // expected-warning{{This lock has been acquired as shared, but exclusive unlock used}} +} + +void bad35(void) { + if (lck_rw_try_lock_exclusive(&rw) != 0) // no-warning + lck_rw_unlock_shared(&rw); // expected-warning{{This lock has been acquired exclusively, but shared unlock used}} +} + +void bad36(void) { + if (lck_rw_try_lock_shared(&rw) != 0) // no-warning + lck_rw_unlock_exclusive(&rw); // expected-warning{{This lock has been acquired as shared, but exclusive unlock used}} +}