https://github.com/gamesh411 created https://github.com/llvm/llvm-project/pull/202473
Bug reports from this checker lack context about where mutexes were locked or unlocked, making it hard to understand the diagnostic without reading surrounding code. Add NoteTag on lock, unlock, destroy, and init events. Notes name the mutex when possible and are filtered to only appear when the mutex is relevant to the reported bug. From 60f2ea6875021060ebe0e3a8beac55271b250f04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Mon, 8 Jun 2026 21:16:12 +0200 Subject: [PATCH] [analyzer] Add path notes to PthreadLockChecker Bug reports from this checker lack context about where mutexes were locked or unlocked, making it hard to understand the diagnostic without reading surrounding code. Add NoteTag on lock, unlock, destroy, and init events. Notes name the mutex when possible and are filtered to only appear when the mutex is relevant to the reported bug. --- clang/docs/ReleaseNotes.rst | 6 + .../Checkers/PthreadLockChecker.cpp | 111 ++++++++++++++---- clang/test/Analysis/pthreadlock-notes.c | 42 +++++++ 3 files changed, 136 insertions(+), 23 deletions(-) create mode 100644 clang/test/Analysis/pthreadlock-notes.c diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index aebd60e1646d6..76223e6f0bb32 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -937,6 +937,12 @@ Crash and bug fixes - Fixed ``security.VAList`` checker producing false positives when analyzing C23 code where ``va_start`` expands to ``__builtin_c23_va_start``. +Improvements +^^^^^^^^^^^^ + +- ``alpha.unix.PthreadLock`` now emits path notes on lock, unlock, destroy, + and init operations. + .. comment: This is for the Static Analyzer. Using the caret `^^^` underlining for subsections: diff --git a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index 1731cb28aa794..7f4a8479a1a15 100644 --- a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -13,8 +13,6 @@ // * FuchsiaLocksChecker, which is also rather similar. // * C11LockChecker which also closely follows Pthread semantics. // -// TODO: Path notes. -// //===----------------------------------------------------------------------===// #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" @@ -202,8 +200,8 @@ class PthreadLockChecker : public Checker<check::PostCall, check::DeadSymbols, const MemRegion *lockR, const SymbolRef *sym) const; void reportBug(CheckerContext &C, std::unique_ptr<BugType> BT[], - const Expr *MtxExpr, CheckerKind CheckKind, - StringRef Desc) const; + const Expr *MtxExpr, const MemRegion *MtxRegion, + CheckerKind CheckKind, StringRef Desc) const; // Init. void InitAnyLock(const CallEvent &Call, CheckerContext &C, @@ -441,11 +439,11 @@ void PthreadLockChecker::AcquireLockAux(const CallEvent &Call, if (const LockState *LState = state->get<LockMap>(lockR)) { if (LState->isLocked()) { - reportBug(C, BT_doublelock, MtxExpr, CheckKind, + reportBug(C, BT_doublelock, MtxExpr, lockR, CheckKind, "This lock has already been acquired"); return; } else if (LState->isDestroyed()) { - reportBug(C, BT_destroylock, MtxExpr, CheckKind, + reportBug(C, BT_destroylock, MtxExpr, lockR, CheckKind, "This lock has already been destroyed"); return; } @@ -492,7 +490,19 @@ void PthreadLockChecker::AcquireLockAux(const CallEvent &Call, // Record that the lock was acquired. lockSucc = lockSucc->add<LockSet>(lockR); lockSucc = lockSucc->set<LockMap>(lockR, LockState::getLocked()); - C.addTransition(lockSucc); + const NoteTag *Note = + C.getNoteTag([lockR](PathSensitiveBugReport &BR, llvm::raw_ostream &OS) { + if (BR.getBugType().getCategory() != "Lock checker") + return; + if (!BR.isInteresting(lockR)) + return; + std::string Name = lockR->getDescriptiveName(); + if (Name.empty()) + OS << "Mutex acquired here"; + else + OS << "Locking " << Name << " here"; + }); + C.addTransition(lockSucc, Note); } void PthreadLockChecker::ReleaseAnyLock(const CallEvent &Call, @@ -519,11 +529,11 @@ void PthreadLockChecker::ReleaseLockAux(const CallEvent &Call, if (const LockState *LState = state->get<LockMap>(lockR)) { if (LState->isUnlocked()) { - reportBug(C, BT_doubleunlock, MtxExpr, CheckKind, + reportBug(C, BT_doubleunlock, MtxExpr, lockR, CheckKind, "This lock has already been unlocked"); return; } else if (LState->isDestroyed()) { - reportBug(C, BT_destroylock, MtxExpr, CheckKind, + reportBug(C, BT_destroylock, MtxExpr, lockR, CheckKind, "This lock has already been destroyed"); return; } @@ -534,9 +544,19 @@ void PthreadLockChecker::ReleaseLockAux(const CallEvent &Call, if (!LS.isEmpty()) { const MemRegion *firstLockR = LS.getHead(); if (firstLockR != lockR) { - reportBug(C, BT_lor, MtxExpr, CheckKind, - "This was not the most recently acquired lock. Possible lock " - "order reversal"); + ExplodedNode *N = C.generateErrorNode(); + if (!N) + return; + initBugType(CheckKind); + auto Report = std::make_unique<PathSensitiveBugReport>( + *BT_lor[CheckKind], + "This was not the most recently acquired lock. Possible lock " + "order reversal", + N); + Report->addRange(MtxExpr->getSourceRange()); + Report->markInteresting(lockR); + Report->markInteresting(firstLockR); + C.emitReport(std::move(Report)); return; } // Record that the lock was released. @@ -544,7 +564,19 @@ void PthreadLockChecker::ReleaseLockAux(const CallEvent &Call, } state = state->set<LockMap>(lockR, LockState::getUnlocked()); - C.addTransition(state); + const NoteTag *Note = + C.getNoteTag([lockR](PathSensitiveBugReport &BR, llvm::raw_ostream &OS) { + if (BR.getBugType().getCategory() != "Lock checker") + return; + if (!BR.isInteresting(lockR)) + return; + std::string Name = lockR->getDescriptiveName(); + if (Name.empty()) + OS << "Mutex released here"; + else + OS << "Unlocking " << Name << " here"; + }); + C.addTransition(state, Note); } void PthreadLockChecker::DestroyPthreadLock(const CallEvent &Call, @@ -587,7 +619,15 @@ void PthreadLockChecker::DestroyLockAux(const CallEvent &Call, SymbolRef sym = Call.getReturnValue().getAsSymbol(); if (!sym) { State = State->remove<LockMap>(LockR); - C.addTransition(State); + const NoteTag *Note = C.getNoteTag( + [LockR](PathSensitiveBugReport &BR, llvm::raw_ostream &OS) { + if (BR.getBugType().getCategory() != "Lock checker") + return; + if (!BR.isInteresting(LockR)) + return; + OS << "Destroying mutex here"; + }); + C.addTransition(State, Note); return; } State = State->set<DestroyRetVal>(LockR, sym); @@ -597,13 +637,29 @@ void PthreadLockChecker::DestroyLockAux(const CallEvent &Call, else State = State->set<LockMap>( LockR, LockState::getUntouchedAndPossiblyDestroyed()); - C.addTransition(State); + const NoteTag *Note = C.getNoteTag( + [LockR](PathSensitiveBugReport &BR, llvm::raw_ostream &OS) { + if (BR.getBugType().getCategory() != "Lock checker") + return; + if (!BR.isInteresting(LockR)) + return; + OS << "Destroying mutex here"; + }); + C.addTransition(State, Note); return; } } else { if (!LState || LState->isUnlocked()) { State = State->set<LockMap>(LockR, LockState::getDestroyed()); - C.addTransition(State); + const NoteTag *Note = C.getNoteTag( + [LockR](PathSensitiveBugReport &BR, llvm::raw_ostream &OS) { + if (BR.getBugType().getCategory() != "Lock checker") + return; + if (!BR.isInteresting(LockR)) + return; + OS << "Destroying mutex here"; + }); + C.addTransition(State, Note); return; } } @@ -612,7 +668,7 @@ void PthreadLockChecker::DestroyLockAux(const CallEvent &Call, ? "This lock is still locked" : "This lock has already been destroyed"; - reportBug(C, BT_destroylock, MtxExpr, CheckKind, Message); + reportBug(C, BT_destroylock, MtxExpr, LockR, CheckKind, Message); } void PthreadLockChecker::InitAnyLock(const CallEvent &Call, CheckerContext &C, @@ -639,7 +695,15 @@ void PthreadLockChecker::InitLockAux(const CallEvent &Call, CheckerContext &C, const struct LockState *LState = State->get<LockMap>(LockR); if (!LState || LState->isDestroyed()) { State = State->set<LockMap>(LockR, LockState::getUnlocked()); - C.addTransition(State); + const NoteTag *Note = C.getNoteTag( + [LockR](PathSensitiveBugReport &BR, llvm::raw_ostream &OS) { + if (BR.getBugType().getCategory() != "Lock checker") + return; + if (!BR.isInteresting(LockR)) + return; + OS << "Initializing mutex here"; + }); + C.addTransition(State, Note); return; } @@ -647,13 +711,12 @@ void PthreadLockChecker::InitLockAux(const CallEvent &Call, CheckerContext &C, ? "This lock is still being held" : "This lock has already been initialized"; - reportBug(C, BT_initlock, MtxExpr, CheckKind, Message); + reportBug(C, BT_initlock, MtxExpr, LockR, CheckKind, Message); } -void PthreadLockChecker::reportBug(CheckerContext &C, - std::unique_ptr<BugType> BT[], - const Expr *MtxExpr, CheckerKind CheckKind, - StringRef Desc) const { +void PthreadLockChecker::reportBug( + CheckerContext &C, std::unique_ptr<BugType> BT[], const Expr *MtxExpr, + const MemRegion *MtxRegion, CheckerKind CheckKind, StringRef Desc) const { ExplodedNode *N = C.generateErrorNode(); if (!N) return; @@ -661,6 +724,8 @@ void PthreadLockChecker::reportBug(CheckerContext &C, auto Report = std::make_unique<PathSensitiveBugReport>(*BT[CheckKind], Desc, N); Report->addRange(MtxExpr->getSourceRange()); + if (MtxRegion) + Report->markInteresting(MtxRegion); C.emitReport(std::move(Report)); } diff --git a/clang/test/Analysis/pthreadlock-notes.c b/clang/test/Analysis/pthreadlock-notes.c new file mode 100644 index 0000000000000..3536457fabe5a --- /dev/null +++ b/clang/test/Analysis/pthreadlock-notes.c @@ -0,0 +1,42 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.unix.PthreadLock \ +// RUN: -analyzer-output text \ +// RUN: -verify %s + +#include "Inputs/system-header-simulator-for-pthread-lock.h" + +pthread_mutex_t mtx; + +void double_lock(void) { + pthread_mutex_lock(&mtx); // expected-note{{Locking 'mtx' here}} + pthread_mutex_lock(&mtx); // expected-warning{{This lock has already been acquired}} + // expected-note@-1{{This lock has already been acquired}} +} + +void double_unlock(void) { + pthread_mutex_lock(&mtx); // expected-note{{Locking 'mtx' here}} + pthread_mutex_unlock(&mtx); // expected-note{{Unlocking 'mtx' here}} + pthread_mutex_unlock(&mtx); // expected-warning{{This lock has already been unlocked}} + // expected-note@-1{{This lock has already been unlocked}} +} + +void use_after_destroy(void) { + pthread_mutex_destroy(&mtx); // expected-note{{Destroying mutex here}} + pthread_mutex_lock(&mtx); // expected-warning{{This lock has already been destroyed}} + // expected-note@-1{{This lock has already been destroyed}} +} + +void double_init(void) { + pthread_mutex_init(&mtx, 0); // expected-note{{Initializing mutex here}} + pthread_mutex_init(&mtx, 0); // expected-warning{{This lock has already been initialized}} + // expected-note@-1{{This lock has already been initialized}} +} + +pthread_mutex_t mtx2; + +void lock_order_reversal(void) { + pthread_mutex_lock(&mtx); // expected-note{{Locking 'mtx' here}} + pthread_mutex_lock(&mtx2); // expected-note{{Locking 'mtx2' here}} + pthread_mutex_unlock(&mtx); // expected-warning{{This was not the most recently acquired lock}} + // expected-note@-1{{This was not the most recently acquired lock}} +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
