Author: Endre Fülöp Date: 2026-06-12T10:08:59+02:00 New Revision: 630f30ca579575632937ff226c0feebb15d03623
URL: https://github.com/llvm/llvm-project/commit/630f30ca579575632937ff226c0feebb15d03623 DIFF: https://github.com/llvm/llvm-project/commit/630f30ca579575632937ff226c0feebb15d03623.diff LOG: [analyzer] Add path notes to PthreadLockChecker (#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. Added: clang/test/Analysis/pthreadlock-notes.c Modified: clang/docs/ReleaseNotes.rst clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index d2ced5ea6791c..a984982d1bd41 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -1001,6 +1001,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..d4f91b03c8d57 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" @@ -24,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 { @@ -202,8 +225,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, @@ -266,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 @@ -441,11 +464,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 +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()); - C.addTransition(lockSucc); + C.addTransition(lockSucc, + createMutexNote(C, lockR, "Locking ", "Mutex acquired here")); } void PthreadLockChecker::ReleaseAnyLock(const CallEvent &Call, @@ -519,11 +543,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 +558,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 +578,8 @@ void PthreadLockChecker::ReleaseLockAux(const CallEvent &Call, } state = state->set<LockMap>(lockR, LockState::getUnlocked()); - C.addTransition(state); + C.addTransition( + state, createMutexNote(C, lockR, "Unlocking ", "Mutex released here")); } void PthreadLockChecker::DestroyPthreadLock(const CallEvent &Call, @@ -587,7 +622,8 @@ void PthreadLockChecker::DestroyLockAux(const CallEvent &Call, SymbolRef sym = Call.getReturnValue().getAsSymbol(); if (!sym) { State = State->remove<LockMap>(LockR); - C.addTransition(State); + C.addTransition(State, createMutexNote(C, LockR, "Destroying ", + "Mutex destroyed here")); return; } State = State->set<DestroyRetVal>(LockR, sym); @@ -597,13 +633,15 @@ void PthreadLockChecker::DestroyLockAux(const CallEvent &Call, else State = State->set<LockMap>( LockR, LockState::getUntouchedAndPossiblyDestroyed()); - C.addTransition(State); + C.addTransition(State, createMutexNote(C, LockR, "Destroying ", + "Mutex destroyed here")); return; } } else { if (!LState || LState->isUnlocked()) { State = State->set<LockMap>(LockR, LockState::getDestroyed()); - C.addTransition(State); + C.addTransition(State, createMutexNote(C, LockR, "Destroying ", + "Mutex destroyed here")); return; } } @@ -612,7 +650,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 +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()); - C.addTransition(State); + C.addTransition(State, createMutexNote(C, LockR, "Initializing ", + "Mutex initialized here")); return; } @@ -647,13 +686,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 +699,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..37796ad03ddd9 --- /dev/null +++ b/clang/test/Analysis/pthreadlock-notes.c @@ -0,0 +1,49 @@ +// 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 '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 'mtx' 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}} +} + + +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
