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,11 @@ bool operator==(const LockState &X) const { return K == X.K; } bool isLocked() const { return K == Locked; } + bool isSharedLocked() const { return K == SharedLocked; } + bool isAnyLocked() const { return isLocked() || isSharedLocked(); } bool isUnlocked() const { return K == Unlocked; } + bool isSharedUnlocked() const { return K == SharedUnlocked; } + bool isAnyUnlocked() const { return isUnlocked() || isSharedUnlocked(); } bool isDestroyed() const { return K == Destroyed; } bool isUntouchedAndPossiblyDestroyed() const { return K == UntouchedAndPossiblyDestroyed; @@ -99,7 +107,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 +115,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 +191,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 +244,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 +259,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 @@ -318,8 +337,12 @@ I.first->dumpToStream(Out); if (I.second.isLocked()) Out << ": locked"; + else if (I.second.isSharedLocked()) + Out << ": locked shared"; else if (I.second.isUnlocked()) Out << ": unlocked"; + else if (I.second.isSharedUnlocked()) + Out << ": unlocked shared"; else if (I.second.isDestroyed()) Out << ": destroyed"; else if (I.second.isUntouchedAndPossiblyDestroyed()) @@ -345,46 +368,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]) @@ -400,7 +438,7 @@ state = resolvePossiblyDestroyedMutex(state, lockR, sym); if (const LockState *LState = state->get(lockR)) { - if (LState->isLocked()) { + if (LState->isAnyLocked()) { reportBug(C, BT_doublelock, MtxExpr, CheckKind, "This lock has already been acquired"); return; @@ -451,19 +489,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; @@ -478,7 +526,7 @@ state = resolvePossiblyDestroyedMutex(state, lockR, sym); if (const LockState *LState = state->get(lockR)) { - if (LState->isUnlocked()) { + if (LState->isAnyUnlocked()) { reportBug(C, BT_doubleunlock, MtxExpr, CheckKind, "This lock has already been unlocked"); return; @@ -486,6 +534,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 +565,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); } @@ -543,7 +607,7 @@ // Checking the return value of the destroy method only in the case of // PthreadSemantics if (Semantics == PthreadSemantics) { - if (!LState || LState->isUnlocked()) { + if (!LState || LState->isAnyUnlocked()) { SymbolRef sym = Call.getReturnValue().getAsSymbol(); if (!sym) { State = State->remove(LockR); @@ -551,7 +615,7 @@ return; } State = State->set(LockR, sym); - if (LState && LState->isUnlocked()) + if (LState && LState->isAnyUnlocked()) State = State->set( LockR, LockState::getUnlockedAndPossiblyDestroyed()); else @@ -561,14 +625,14 @@ return; } } else { - if (!LState || LState->isUnlocked()) { + if (!LState || LState->isAnyUnlocked()) { State = State->set(LockR, LockState::getDestroyed()); C.addTransition(State); return; } } - StringRef Message = LState->isLocked() + StringRef Message = LState->isAnyLocked() ? "This lock is still locked" : "This lock has already been destroyed"; @@ -603,7 +667,7 @@ return; } - StringRef Message = LState->isLocked() + StringRef Message = LState->isAnyLocked() ? "This lock is still being held" : "This lock has already been initialized"; 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 @@ -34,6 +34,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 *); +extern boolean_t lck_rw_try_lock_exclusive(lck_rw_t *); 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_unlock_shared(&rw); // no-warning +} + void bad1(void) { @@ -502,14 +512,47 @@ } void bad32(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_shared(&rw); - lck_rw_unlock_exclusive(&rw); // FIXME: warn - should be shared? + lck_rw_lock_shared(&rw); // expected-warning{{This lock has already been acquired}} +} + +void bad34(void) { lck_rw_lock_exclusive(&rw); - lck_rw_unlock_shared(&rw); // FIXME: warn - should be exclusive? + lck_rw_lock_exclusive(&rw); // expected-warning{{This lock has already been acquired}} } -void bad33(void) { - pthread_mutex_lock(pmtx); - fake_system_function(); - pthread_mutex_lock(pmtx); // expected-warning{{This lock has already been acquired}} +void bad35(void) { + lck_rw_lock_exclusive(&rw); + lck_rw_lock_shared(&rw); // expected-warning{{This lock has already been acquired}} +} + +void bad36(void) { + lck_rw_lock_shared(&rw); + lck_rw_lock_exclusive(&rw); // expected-warning{{This lock has already been acquired}} +} + +void bad37(void) { + lck_rw_lock_exclusive(&rw); + lck_rw_unlock_shared(&rw); // expected-warning{{This lock has been acquired exclusively, but shared unlock used}} +} + +void bad38(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 bad39(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 bad40(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}} }