https://github.com/gamesh411 updated https://github.com/llvm/llvm-project/pull/202473
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 1/2] [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}} +} From 60793f9c7aa13462421bec41c72b513a442aa90b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Thu, 11 Jun 2026 15:11:24 +0200 Subject: [PATCH 2/2] factor out note creation logic to reduce duplication, note creation and interestingness checks are factored out. also the checker category string is no longer a magic string --- .../Checkers/PthreadLockChecker.cpp | 115 +++++++----------- clang/test/Analysis/pthreadlock-notes.c | 11 +- 2 files changed, 54 insertions(+), 72 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index 7f4a8479a1a15..d4f91b03c8d57 100644 --- a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -22,10 +22,35 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "llvm/ADT/StringRef.h" using namespace clang; using namespace ento; +constexpr llvm::StringRef LOCK_CHECKER_CATEGORY = "Lock checker"; + +static bool isLockRelevant(const MemRegion *R, + const PathSensitiveBugReport &BR) { + return BR.getBugType().getCategory() == LOCK_CHECKER_CATEGORY && + BR.isInteresting(R); +} + +static const NoteTag *createMutexNote(CheckerContext &C, const MemRegion *R, + StringRef MsgForNamed, + StringRef MsgForUnnamed) { + return C.getNoteTag( + [R, Named = MsgForNamed.str(), Unnamed = MsgForUnnamed.str()]( + PathSensitiveBugReport &BR, llvm::raw_ostream &OS) { + if (!isLockRelevant(R, BR)) + return; + std::string Name = R->getDescriptiveName(); + if (Name.empty()) + OS << Unnamed; + else + OS << Named << Name << " here"; + }); +} + namespace { struct LockState { @@ -264,16 +289,16 @@ class PthreadLockChecker : public Checker<check::PostCall, check::DeadSymbols, void initBugType(CheckerKind CheckKind) const { if (BT_doublelock[CheckKind]) return; - BT_doublelock[CheckKind].reset( - new BugType{CheckNames[CheckKind], "Double locking", "Lock checker"}); - BT_doubleunlock[CheckKind].reset( - new BugType{CheckNames[CheckKind], "Double unlocking", "Lock checker"}); + BT_doublelock[CheckKind].reset(new BugType{ + CheckNames[CheckKind], "Double locking", LOCK_CHECKER_CATEGORY}); + BT_doubleunlock[CheckKind].reset(new BugType{ + CheckNames[CheckKind], "Double unlocking", LOCK_CHECKER_CATEGORY}); BT_destroylock[CheckKind].reset(new BugType{ - CheckNames[CheckKind], "Use destroyed lock", "Lock checker"}); + CheckNames[CheckKind], "Use destroyed lock", LOCK_CHECKER_CATEGORY}); BT_initlock[CheckKind].reset(new BugType{ - CheckNames[CheckKind], "Init invalid lock", "Lock checker"}); - BT_lor[CheckKind].reset(new BugType{CheckNames[CheckKind], - "Lock order reversal", "Lock checker"}); + CheckNames[CheckKind], "Init invalid lock", LOCK_CHECKER_CATEGORY}); + BT_lor[CheckKind].reset(new BugType{ + CheckNames[CheckKind], "Lock order reversal", LOCK_CHECKER_CATEGORY}); } }; } // end anonymous namespace @@ -490,19 +515,8 @@ void PthreadLockChecker::AcquireLockAux(const CallEvent &Call, // Record that the lock was acquired. lockSucc = lockSucc->add<LockSet>(lockR); lockSucc = lockSucc->set<LockMap>(lockR, LockState::getLocked()); - 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); + C.addTransition(lockSucc, + createMutexNote(C, lockR, "Locking ", "Mutex acquired here")); } void PthreadLockChecker::ReleaseAnyLock(const CallEvent &Call, @@ -564,19 +578,8 @@ void PthreadLockChecker::ReleaseLockAux(const CallEvent &Call, } state = state->set<LockMap>(lockR, LockState::getUnlocked()); - 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); + C.addTransition( + state, createMutexNote(C, lockR, "Unlocking ", "Mutex released here")); } void PthreadLockChecker::DestroyPthreadLock(const CallEvent &Call, @@ -619,15 +622,8 @@ void PthreadLockChecker::DestroyLockAux(const CallEvent &Call, SymbolRef sym = Call.getReturnValue().getAsSymbol(); if (!sym) { State = State->remove<LockMap>(LockR); - 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); + C.addTransition(State, createMutexNote(C, LockR, "Destroying ", + "Mutex destroyed here")); return; } State = State->set<DestroyRetVal>(LockR, sym); @@ -637,29 +633,15 @@ void PthreadLockChecker::DestroyLockAux(const CallEvent &Call, else State = State->set<LockMap>( LockR, LockState::getUntouchedAndPossiblyDestroyed()); - 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); + C.addTransition(State, createMutexNote(C, LockR, "Destroying ", + "Mutex destroyed here")); return; } } else { if (!LState || LState->isUnlocked()) { State = State->set<LockMap>(LockR, LockState::getDestroyed()); - 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); + C.addTransition(State, createMutexNote(C, LockR, "Destroying ", + "Mutex destroyed here")); return; } } @@ -695,15 +677,8 @@ 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()); - 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); + C.addTransition(State, createMutexNote(C, LockR, "Initializing ", + "Mutex initialized here")); return; } diff --git a/clang/test/Analysis/pthreadlock-notes.c b/clang/test/Analysis/pthreadlock-notes.c index 3536457fabe5a..37796ad03ddd9 100644 --- a/clang/test/Analysis/pthreadlock-notes.c +++ b/clang/test/Analysis/pthreadlock-notes.c @@ -21,13 +21,13 @@ void double_unlock(void) { } void use_after_destroy(void) { - pthread_mutex_destroy(&mtx); // expected-note{{Destroying mutex here}} + pthread_mutex_destroy(&mtx); // expected-note{{Destroying 'mtx' 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-note{{Initializing 'mtx' here}} pthread_mutex_init(&mtx, 0); // expected-warning{{This lock has already been initialized}} // expected-note@-1{{This lock has already been initialized}} } @@ -40,3 +40,10 @@ void lock_order_reversal(void) { 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}} } + + +void double_lock_via_param(pthread_mutex_t *m) { + pthread_mutex_lock(m); // expected-note{{Mutex acquired here}} + pthread_mutex_lock(m); // expected-warning{{This lock has already been acquired}} + // expected-note@-1{{This lock has already been acquired}} +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
