https://github.com/gamesh411 updated https://github.com/llvm/llvm-project/pull/202452
From 4352fd64c32319600ee8d7cefd170a460a2ff8ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Mon, 8 Jun 2026 20:54:34 +0200 Subject: [PATCH 1/6] [analyzer] Disable lock order reversal check by default in PthreadLockChecker Lock order reversal is a real source of deadlocks, but the current single-path intraprocedural analysis is a single-path analysis, and it cannot reason about potentially overlapping executions.This makes this part of the checker too imprecise for default-on. Add a WarnOnLockOrderReversal option (default: false) for the previous behavior. --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 7 ++++ .../Checkers/PthreadLockChecker.cpp | 27 ++++++++++--- clang/test/Analysis/c11lock.c | 11 +++++- clang/test/Analysis/fuchsia_lock.c | 13 +++++-- clang/test/Analysis/pthreadlock.c | 38 +++++++++++++++---- 5 files changed, 79 insertions(+), 17 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index eca2afbe340a9..a81a19063b168 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -594,6 +594,13 @@ let ParentPackage = UnixAlpha in { def PthreadLockChecker : Checker<"PthreadLock">, HelpText<"Simple lock -> unlock checker">, + CheckerOptions<[ + CmdLineOption<Boolean, + "WarnOnLockOrderReversal", + "Warn when locks are not released in LIFO order.", + "false", + Released> + ]>, Dependencies<[PthreadLockBase]>, Documentation<HasDocumentation>; diff --git a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index 1731cb28aa794..41abd9b985589 100644 --- a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -80,6 +80,7 @@ class PthreadLockChecker : public Checker<check::PostCall, check::DeadSymbols, }; bool ChecksEnabled[CK_NumCheckKinds] = {false}; CheckerNameRef CheckNames[CK_NumCheckKinds]; + bool WarnOnLockOrderReversal = false; private: typedef void (PthreadLockChecker::*FnCheck)(const CallEvent &Call, @@ -532,15 +533,18 @@ void PthreadLockChecker::ReleaseLockAux(const CallEvent &Call, LockSetTy LS = state->get<LockSet>(); if (!LS.isEmpty()) { - const MemRegion *firstLockR = LS.getHead(); - if (firstLockR != lockR) { + if (WarnOnLockOrderReversal && LS.getHead() != lockR) { reportBug(C, BT_lor, MtxExpr, CheckKind, "This was not the most recently acquired lock. Possible lock " "order reversal"); return; } - // Record that the lock was released. - state = state->set<LockSet>(LS.getTail()); + auto &Factory = state->get_context<LockSet>(); + llvm::ImmutableList<const MemRegion *> NewLS = Factory.getEmptyList(); + for (auto I = LS.begin(), E = LS.end(); I != E; ++I) + if (*I != lockR) + NewLS = Factory.add(*I, NewLS); + state = state->set<LockSet>(NewLS); } state = state->set<LockMap>(lockR, LockState::getUnlocked()); @@ -742,8 +746,21 @@ bool ento::shouldRegisterPthreadLockBase(const CheckerManager &mgr) { return tru \ bool ento::shouldRegister##name(const CheckerManager &mgr) { return true; } -REGISTER_CHECKER(PthreadLockChecker) REGISTER_CHECKER(FuchsiaLockChecker) REGISTER_CHECKER(C11LockChecker) #undef REGISTER_CHECKER + +void ento::registerPthreadLockChecker(CheckerManager &mgr) { + PthreadLockChecker *checker = mgr.getChecker<PthreadLockChecker>(); + checker->ChecksEnabled[PthreadLockChecker::CK_PthreadLockChecker] = true; + checker->CheckNames[PthreadLockChecker::CK_PthreadLockChecker] = + mgr.getCurrentCheckerName(); + checker->WarnOnLockOrderReversal = + mgr.getAnalyzerOptions().getCheckerBooleanOption( + mgr.getCurrentCheckerName(), "WarnOnLockOrderReversal"); +} + +bool ento::shouldRegisterPthreadLockChecker(const CheckerManager &mgr) { + return true; +} diff --git a/clang/test/Analysis/c11lock.c b/clang/test/Analysis/c11lock.c index 0e867e9dada36..29ebcd5f37c57 100644 --- a/clang/test/Analysis/c11lock.c +++ b/clang/test/Analysis/c11lock.c @@ -1,4 +1,11 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.C11Lock -verify %s +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.core.C11Lock \ +// RUN: -verify %s +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.core.C11Lock \ +// RUN: -analyzer-checker=alpha.unix.PthreadLock \ +// RUN: -analyzer-config alpha.unix.PthreadLock:WarnOnLockOrderReversal=true \ +// RUN: -verify=expected,lor %s typedef int mtx_t; struct timespec; @@ -61,7 +68,7 @@ void bad6(void) { void bad7(void) { mtx_lock(&mtx1); mtx_lock(&mtx2); - mtx_unlock(&mtx1); // expected-warning {{This was not the most recently acquired lock. Possible lock order reversal}} + mtx_unlock(&mtx1); // lor-warning {{This was not the most recently acquired lock. Possible lock order reversal}} mtx_unlock(&mtx2); } diff --git a/clang/test/Analysis/fuchsia_lock.c b/clang/test/Analysis/fuchsia_lock.c index 2067b606f7517..1210ca6942150 100644 --- a/clang/test/Analysis/fuchsia_lock.c +++ b/clang/test/Analysis/fuchsia_lock.c @@ -1,4 +1,11 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.fuchsia.Lock -verify %s +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.fuchsia.Lock \ +// RUN: -verify %s +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.fuchsia.Lock \ +// RUN: -analyzer-checker=alpha.unix.PthreadLock \ +// RUN: -analyzer-config alpha.unix.PthreadLock:WarnOnLockOrderReversal=true \ +// RUN: -verify=expected,lor %s typedef int spin_lock_t; typedef int zx_status_t; @@ -38,7 +45,7 @@ void bad3(void) { void bad4(void) { spin_lock(&mtx1); spin_lock(&mtx2); - spin_unlock(&mtx1); // expected-warning {{This was not the most recently acquired lock. Possible lock order reversal}} + spin_unlock(&mtx1); // lor-warning {{This was not the most recently acquired lock. Possible lock order reversal}} spin_unlock(&mtx2); } @@ -87,7 +94,7 @@ void bad13(void) { void bad14(void) { sync_mutex_lock(&smtx1); sync_mutex_lock(&smtx2); - sync_mutex_unlock(&smtx1); // expected-warning {{This was not the most recently acquired lock. Possible lock order reversal}} + sync_mutex_unlock(&smtx1); // lor-warning {{This was not the most recently acquired lock. Possible lock order reversal}} sync_mutex_unlock(&smtx2); } diff --git a/clang/test/Analysis/pthreadlock.c b/clang/test/Analysis/pthreadlock.c index 85b34cbed918d..e931569c45ab8 100644 --- a/clang/test/Analysis/pthreadlock.c +++ b/clang/test/Analysis/pthreadlock.c @@ -1,4 +1,10 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.PthreadLock -verify %s +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.unix.PthreadLock \ +// RUN: -verify %s +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.unix.PthreadLock \ +// RUN: -analyzer-config alpha.unix.PthreadLock:WarnOnLockOrderReversal=true \ +// RUN: -verify=expected,lor %s // Tests performing normal locking patterns and wrong locking orders @@ -263,8 +269,8 @@ bad3(void) { pthread_mutex_lock(&mtx1); // no-warning pthread_mutex_lock(&mtx2); // no-warning - pthread_mutex_unlock(&mtx1); // expected-warning{{This was not the most recently acquired lock}} - pthread_mutex_unlock(&mtx2); + pthread_mutex_unlock(&mtx1); // lor-warning{{This was not the most recently acquired lock. Possible lock order reversal}} + pthread_mutex_unlock(&mtx2); // no-warning } void @@ -273,7 +279,7 @@ bad4(void) if (pthread_mutex_trylock(&mtx1)) // no-warning return; pthread_mutex_lock(&mtx2); // no-warning - pthread_mutex_unlock(&mtx1); // expected-warning{{This was not the most recently acquired lock}} + pthread_mutex_unlock(&mtx1); // lor-warning{{This was not the most recently acquired lock. Possible lock order reversal}} } void @@ -297,8 +303,8 @@ bad7(void) { lck_mtx_lock(&lck1); // no-warning lck_mtx_lock(&lck2); // no-warning - lck_mtx_unlock(&lck1); // expected-warning{{This was not the most recently acquired lock}} - lck_mtx_unlock(&lck2); + lck_mtx_unlock(&lck1); // lor-warning{{This was not the most recently acquired lock. Possible lock order reversal}} + lck_mtx_unlock(&lck2); // no-warning } void @@ -307,7 +313,7 @@ bad8(void) if (lck_mtx_try_lock(&lck1) == 0) // no-warning return; lck_mtx_lock(&lck2); // no-warning - lck_mtx_unlock(&lck1); // expected-warning{{This was not the most recently acquired lock}} + lck_mtx_unlock(&lck1); // lor-warning{{This was not the most recently acquired lock. Possible lock order reversal}} } void @@ -519,3 +525,21 @@ void nocrash1(pthread_mutex_t *mutex) { if (ret == 0) // no crash ; } + +pthread_mutex_t mtx3; + +void ok_non_lifo_unlock_three(void) { + pthread_mutex_lock(&mtx1); // no-warning + pthread_mutex_lock(&mtx2); // no-warning + pthread_mutex_lock(&mtx3); // no-warning + pthread_mutex_unlock(&mtx2); // lor-warning{{This was not the most recently acquired lock. Possible lock order reversal}} + pthread_mutex_unlock(&mtx3); // no-warning + pthread_mutex_unlock(&mtx1); // no-warning +} + +void ok_conditional_lock(int started) { + if (started) + pthread_mutex_lock(&mtx1); + // On the !started path, no lock was acquired. + pthread_mutex_unlock(&mtx1); // no-warning +} From b35bdf70446fccea8ff087643018c46b280a7525 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Tue, 9 Jun 2026 01:08:05 +0200 Subject: [PATCH 2/6] add docs and tests for WarnOnLockOrderReversal option --- clang/docs/ReleaseNotes.rst | 7 +++++++ clang/docs/analyzer/checkers.rst | 8 ++++++++ clang/test/Analysis/analyzer-config.c | 1 + 3 files changed, 16 insertions(+) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index aebd60e1646d6..a69203bb0d7c1 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -937,6 +937,13 @@ 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 +^^^^^^^^^^^^ + +- The lock-order-reversal check in ``alpha.unix.PthreadLock`` is now disabled + by default. It can be re-enabled with + ``-analyzer-config alpha.unix.PthreadLock:WarnOnLockOrderReversal=true``. + .. comment: This is for the Static Analyzer. Using the caret `^^^` underlining for subsections: diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 61f591916018e..ee700b71eb3f8 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -3609,6 +3609,12 @@ Applies to: ``pthread_mutex_lock, pthread_rwlock_rdlock, pthread_rwlock_wrlock, ``lck_rw_lock_shared, pthread_mutex_trylock, pthread_rwlock_tryrdlock, pthread_rwlock_tryrwlock, lck_mtx_try_lock, lck_rw_try_lock_exclusive, lck_rw_try_lock_shared, pthread_mutex_unlock, pthread_rwlock_unlock, lck_mtx_unlock, lck_rw_done``. +**Options** + +* ``WarnOnLockOrderReversal`` (boolean). If set to true, the checker will warn + on non-LIFO unlock order (possible lock order reversal). Defaults to false + because detecting real lock order violations requires cross-path analysis of + acquisition order, which the analyzer's single-path engine does not support. .. code-block:: c @@ -3620,6 +3626,8 @@ lck_rw_try_lock_exclusive, lck_rw_try_lock_shared, pthread_mutex_unlock, pthread // warn: this lock has already been acquired } + // The following warnings require WarnOnLockOrderReversal=true: + lck_mtx_t lck1, lck2; void test() { diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c index 04dc8c24421bc..8b11d1e74e020 100644 --- a/clang/test/Analysis/analyzer-config.c +++ b/clang/test/Analysis/analyzer-config.c @@ -9,6 +9,7 @@ // CHECK-NEXT: alpha.clone.CloneChecker:ReportNormalClones = true // CHECK-NEXT: alpha.cplusplus.STLAlgorithmModeling:AggressiveStdFindModeling = false // CHECK-NEXT: alpha.osx.cocoa.DirectIvarAssignment:AnnotatedFunctions = false +// CHECK-NEXT: alpha.unix.PthreadLock:WarnOnLockOrderReversal = false // CHECK-NEXT: apply-fixits = false // CHECK-NEXT: assume-at-least-one-iteration = false // CHECK-NEXT: assume-controlled-environment = false From d8325c01ddba3c6744e7c54b0e819ff90a4d4d70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Thu, 11 Jun 2026 17:29:26 +0200 Subject: [PATCH 3/6] link WarnOnLockOrderReversal to checker docs in release notes --- clang/docs/ReleaseNotes.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index b353211eb1911..659c43620d912 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -1001,9 +1001,9 @@ Crash and bug fixes Improvements ^^^^^^^^^^^^ -- The lock-order-reversal check in ``alpha.unix.PthreadLock`` is now disabled - by default. It can be re-enabled with - ``-analyzer-config alpha.unix.PthreadLock:WarnOnLockOrderReversal=true``. +- The lock-order-reversal check in ``alpha.unix.PthreadLock`` is now disabled by default. + It can be re-enabled with the + :ref:`WarnOnLockOrderReversal <alpha-unix-PthreadLock>` option. .. comment: This is for the Static Analyzer. From 769931575eeddaff7c0200dd169c90aaecd0bf07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Thu, 11 Jun 2026 17:29:30 +0200 Subject: [PATCH 4/6] enable PthreadLock in c11lock.c first RUN to test off by default run as well --- clang/test/Analysis/c11lock.c | 1 + 1 file changed, 1 insertion(+) diff --git a/clang/test/Analysis/c11lock.c b/clang/test/Analysis/c11lock.c index 29ebcd5f37c57..d04603c7e2cc1 100644 --- a/clang/test/Analysis/c11lock.c +++ b/clang/test/Analysis/c11lock.c @@ -1,5 +1,6 @@ // RUN: %clang_analyze_cc1 \ // RUN: -analyzer-checker=alpha.core.C11Lock \ +// RUN: -analyzer-checker=alpha.unix.PthreadLock \ // RUN: -verify %s // RUN: %clang_analyze_cc1 \ // RUN: -analyzer-checker=alpha.core.C11Lock \ From cbd06be2223a154376613a9be4237350dcfd859c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Thu, 11 Jun 2026 17:30:51 +0200 Subject: [PATCH 5/6] use make_filter_range for LockSet rebuild --- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index 41abd9b985589..f204a57345418 100644 --- a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -24,6 +24,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "llvm/ADT/STLExtras.h" using namespace clang; using namespace ento; @@ -541,9 +542,10 @@ void PthreadLockChecker::ReleaseLockAux(const CallEvent &Call, } auto &Factory = state->get_context<LockSet>(); llvm::ImmutableList<const MemRegion *> NewLS = Factory.getEmptyList(); - for (auto I = LS.begin(), E = LS.end(); I != E; ++I) - if (*I != lockR) - NewLS = Factory.add(*I, NewLS); + for (const MemRegion *LockReg : + llvm::make_filter_range(LS, llvm::not_equal_to(lockR))) { + NewLS = Factory.add(LockReg, NewLS); + } state = state->set<LockSet>(NewLS); } From d2c9788ac9c6c3cbff1f00cdc02c90f84362d0e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Thu, 11 Jun 2026 17:30:55 +0200 Subject: [PATCH 6/6] use LLVM naming convention --- .../Checkers/PthreadLockChecker.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index f204a57345418..d3abcbfa37d2d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -753,16 +753,16 @@ REGISTER_CHECKER(C11LockChecker) #undef REGISTER_CHECKER -void ento::registerPthreadLockChecker(CheckerManager &mgr) { - PthreadLockChecker *checker = mgr.getChecker<PthreadLockChecker>(); - checker->ChecksEnabled[PthreadLockChecker::CK_PthreadLockChecker] = true; - checker->CheckNames[PthreadLockChecker::CK_PthreadLockChecker] = - mgr.getCurrentCheckerName(); - checker->WarnOnLockOrderReversal = - mgr.getAnalyzerOptions().getCheckerBooleanOption( - mgr.getCurrentCheckerName(), "WarnOnLockOrderReversal"); +void ento::registerPthreadLockChecker(CheckerManager &Mgr) { + PthreadLockChecker *Checker = Mgr.getChecker<PthreadLockChecker>(); + Checker->ChecksEnabled[PthreadLockChecker::CK_PthreadLockChecker] = true; + Checker->CheckNames[PthreadLockChecker::CK_PthreadLockChecker] = + Mgr.getCurrentCheckerName(); + Checker->WarnOnLockOrderReversal = + Mgr.getAnalyzerOptions().getCheckerBooleanOption( + Mgr.getCurrentCheckerName(), "WarnOnLockOrderReversal"); } -bool ento::shouldRegisterPthreadLockChecker(const CheckerManager &mgr) { +bool ento::shouldRegisterPthreadLockChecker(const CheckerManager &) { return true; } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
