ASDenysPetrov updated this revision to Diff 290133.
ASDenysPetrov added a comment.
Minor fixes.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87146/new/
https://reviews.llvm.org/D87146
Files:
clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
clang/test/Analysis/pthreadlock.c
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,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}}
}
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/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<BugType> BT_destroylock[CK_NumCheckKinds];
mutable std::unique_ptr<BugType> BT_initlock[CK_NumCheckKinds];
mutable std::unique_ptr<BugType> BT_lor[CK_NumCheckKinds];
+ mutable std::unique_ptr<BugType> 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<LockMap>(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<LockSet>(lockR);
- lockSucc = lockSucc->set<LockMap>(lockR, LockState::getLocked());
+ auto State = IsShared ? LockState::getSharedLocked() : LockState::getLocked();
+ lockSucc = lockSucc->set<LockMap>(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<LockMap>(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<LockSet>(LS.getTail());
}
- state = state->set<LockMap>(lockR, LockState::getUnlocked());
+ auto State =
+ IsShared ? LockState::getSharedUnlocked() : LockState::getUnlocked();
+ state = state->set<LockMap>(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<LockMap>(LockR);
@@ -551,7 +615,7 @@
return;
}
State = State->set<DestroyRetVal>(LockR, sym);
- if (LState && LState->isUnlocked())
+ if (LState && LState->isAnyUnlocked())
State = State->set<LockMap>(
LockR, LockState::getUnlockedAndPossiblyDestroyed());
else
@@ -561,14 +625,14 @@
return;
}
} else {
- if (!LState || LState->isUnlocked()) {
+ if (!LState || LState->isAnyUnlocked()) {
State = State->set<LockMap>(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";
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
