[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
balazske wrote: It looks like that this change causes crashes on many projects (curl, vim, postgres, others) in `RAIIMutexDescriptor::initIdentifierInfo`. https://github.com/llvm/llvm-project/pull/80029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
https://github.com/gamesh411 closed https://github.com/llvm/llvm-project/pull/80029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
Endre =?utf-8?q?Fülöp?= ,
Endre =?utf-8?q?Fülöp?=
Message-ID:
In-Reply-To:
@@ -20,48 +20,178 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class CallDescriptionBasedMatcher {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ CallDescriptionBasedMatcher(CallDescription &&LockFn,
+ CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matches(const CallEvent &Call, bool IsLock) const {
+if (IsLock) {
+ return LockFn.matches(Call);
+}
NagyDonat wrote:
```suggestion
if (IsLock)
return LockFn.matches(Call);
```
Bikeshedding: the coding standard says that braces shouldn't be used on simple
single-statement bodies:
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
Unorthodox but (IMO) elegant alternative idea: replace the whole body of the
function with
```
return (isLock ? LockFn : UnlockFn).matches(Call);
```
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
Endre =?utf-8?q?Fülöp?= ,
Endre =?utf-8?q?Fülöp?=
Message-ID:
In-Reply-To:
@@ -20,48 +20,178 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class CallDescriptionBasedMatcher {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ CallDescriptionBasedMatcher(CallDescription &&LockFn,
+ CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matches(const CallEvent &Call, bool IsLock) const {
+if (IsLock) {
+ return LockFn.matches(Call);
+}
+return UnlockFn.matches(Call);
+ }
+};
+
+class FirstArgMutexDescriptor : public CallDescriptionBasedMatcher {
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : CallDescriptionBasedMatcher(std::move(LockFn), std::move(UnlockFn)) {}
+
+ [[nodiscard]] const MemRegion *getRegion(const CallEvent &Call, bool) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor : public CallDescriptionBasedMatcher {
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : CallDescriptionBasedMatcher(std::move(LockFn), std::move(UnlockFn)) {}
+
+ [[nodiscard]] const MemRegion *getRegion(const CallEvent &Call, bool) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
+
+class RAIIMutexDescriptor {
+ mutable const IdentifierInfo *Guard{};
+ mutable bool IdentifierInfoInitialized{};
+ mutable llvm::SmallString<32> GuardName{};
+
+ void initIdentifierInfo(const CallEvent &Call) const {
+if (!IdentifierInfoInitialized) {
+ // In case of checking C code, or when the corresponding headers are not
+ // included, we might end up query the identifier table every time when
+ // this function is called instead of early returning it. To avoid this,
a
+ // bool variable (IdentifierInfoInitialized) is used and the function
will
+ // be run only once.
+ Guard = &Call.getCalleeAnalysisDeclContext()->getASTContext().Idents.get(
+ GuardName);
+ IdentifierInfoInitialized = true;
+}
+ }
+
+ template bool matchesImpl(const CallEvent &Call) const {
+const T *C = dyn_cast(&Call);
+if (!C)
+ return false;
+const IdentifierInfo *II =
+cast(C->getDecl()->getParent())->getIdentifier();
+return II == Guard;
+ }
+
+public:
+ RAIIMutexDescriptor(StringRef GuardName) : GuardName(GuardName) {}
+ [[nodiscard]] bool matches(const CallEvent &Call, bool IsLock) const {
+initIdentifierInfo(Call);
+if (IsLock) {
+ return matchesImpl(Call);
+}
+return matchesImpl(Call);
+ }
+ [[nodiscard]] const MemRegion *getRegion(const CallEvent &Call,
+ bool IsLock) const {
+const MemRegion *LockRegion = nullptr;
+if (IsLock) {
+ if (std::optional Object = Call.getReturnValueUnderConstruction())
{
+LockRegion = Object->getAsRegion();
+ }
NagyDonat wrote:
```suggestion
if (std::optional Object = Call.getReturnValueUnderConstruction())
LockRegion = Object->getAsRegion();
```
Yet another brace tweak...
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
Endre =?utf-8?q?Fülöp?= ,
Endre =?utf-8?q?Fülöp?=
Message-ID:
In-Reply-To:
@@ -20,48 +20,178 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class CallDescriptionBasedMatcher {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ CallDescriptionBasedMatcher(CallDescription &&LockFn,
+ CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matches(const CallEvent &Call, bool IsLock) const {
+if (IsLock) {
+ return LockFn.matches(Call);
+}
+return UnlockFn.matches(Call);
+ }
+};
+
+class FirstArgMutexDescriptor : public CallDescriptionBasedMatcher {
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : CallDescriptionBasedMatcher(std::move(LockFn), std::move(UnlockFn)) {}
+
+ [[nodiscard]] const MemRegion *getRegion(const CallEvent &Call, bool) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor : public CallDescriptionBasedMatcher {
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : CallDescriptionBasedMatcher(std::move(LockFn), std::move(UnlockFn)) {}
+
+ [[nodiscard]] const MemRegion *getRegion(const CallEvent &Call, bool) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
+
+class RAIIMutexDescriptor {
+ mutable const IdentifierInfo *Guard{};
+ mutable bool IdentifierInfoInitialized{};
+ mutable llvm::SmallString<32> GuardName{};
+
+ void initIdentifierInfo(const CallEvent &Call) const {
+if (!IdentifierInfoInitialized) {
+ // In case of checking C code, or when the corresponding headers are not
+ // included, we might end up query the identifier table every time when
+ // this function is called instead of early returning it. To avoid this,
a
+ // bool variable (IdentifierInfoInitialized) is used and the function
will
+ // be run only once.
+ Guard = &Call.getCalleeAnalysisDeclContext()->getASTContext().Idents.get(
+ GuardName);
+ IdentifierInfoInitialized = true;
+}
+ }
+
+ template bool matchesImpl(const CallEvent &Call) const {
+const T *C = dyn_cast(&Call);
+if (!C)
+ return false;
+const IdentifierInfo *II =
+cast(C->getDecl()->getParent())->getIdentifier();
+return II == Guard;
+ }
+
+public:
+ RAIIMutexDescriptor(StringRef GuardName) : GuardName(GuardName) {}
+ [[nodiscard]] bool matches(const CallEvent &Call, bool IsLock) const {
+initIdentifierInfo(Call);
+if (IsLock) {
+ return matchesImpl(Call);
+}
NagyDonat wrote:
```suggestion
if (IsLock)
return matchesImpl(Call);
```
Bikeshedding as above.
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
Endre =?utf-8?q?Fülöp?= , Endre =?utf-8?q?Fülöp?= Message-ID: In-Reply-To: https://github.com/NagyDonat approved this pull request. Thanks for simplifying the code :) I added some stereotypical bikeshedding in inline comments, but apart from that the commit LGTM. https://github.com/llvm/llvm-project/pull/80029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
Endre =?utf-8?q?Fülöp?= , Endre =?utf-8?q?Fülöp?= Message-ID: In-Reply-To: https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/80029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
https://github.com/gamesh411 updated
https://github.com/llvm/llvm-project/pull/80029
From 346e2296869e750c7ec5bd75cf05f80a23b70569 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?=
Date: Tue, 30 Jan 2024 11:33:30 +0100
Subject: [PATCH 1/3] [clang][analyzer] Improve BlockInCriticalSectionsChecker
with multi-section and recursive mutex support
* Add support for multiple, potentially overlapping critical sections:
The checker can now simultaneously handle several mutex's critical
sections without confusing them.
* Implement the handling of recursive mutexes:
By identifying the lock events, recursive mutexes are now supported.
A lock event is a pair of a lock expression and the SVal of the mutex
that it locks, so even multiple locks of the same mutex (and even by
the same expression) is now supported.
* Refine the note tags generated by the checker:
The note tags now correctly show just for mutexes those are
active at point of error, and multiple acqisitions of the same mutex
are also noted.
---
.../BlockInCriticalSectionChecker.cpp | 391 ++
.../Analysis/block-in-critical-section.cpp| 270 +---
2 files changed, 515 insertions(+), 146 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 66e080adb1382b..74ec4b73bd8b5f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -20,48 +20,180 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class FirstArgMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call);
+ }
+ bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call);
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
+
+class RAIIMutexDescriptor {
+ mutable const IdentifierInfo *Guard{};
+ mutable bool IdentifierInfoInitialized{};
+ mutable llvm::SmallString<32> GuardName{};
+
+ void initIdentifierInfo(const CallEvent &Call) const {
+if (!IdentifierInfoInitialized) {
+ // In case of checking C code, or when the corresponding headers are not
+ // included, we might end up query the identifier table every time when
+ // this function is called instead of early returning it. To avoid this,
a
+ // bool variable (IdentifierInfoInitialized) is used and the function
will
+ // be run only once.
+ Guard = &Call.getCalleeAnalysisDeclContext()->getASTContext().Idents.get(
+ GuardName);
+ IdentifierInfoInitialized = true;
+}
+ }
+
+pu
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
@@ -20,48 +20,180 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class FirstArgMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call);
+ }
+ bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call);
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
gamesh411 wrote:
Introduced a common base class for these 2 classes.
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
@@ -20,48 +20,180 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class FirstArgMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call);
+ }
+ bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call);
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
+
+class RAIIMutexDescriptor {
+ mutable const IdentifierInfo *Guard{};
+ mutable bool IdentifierInfoInitialized{};
+ mutable llvm::SmallString<32> GuardName{};
+
+ void initIdentifierInfo(const CallEvent &Call) const {
+if (!IdentifierInfoInitialized) {
+ // In case of checking C code, or when the corresponding headers are not
+ // included, we might end up query the identifier table every time when
+ // this function is called instead of early returning it. To avoid this,
a
+ // bool variable (IdentifierInfoInitialized) is used and the function
will
+ // be run only once.
+ Guard = &Call.getCalleeAnalysisDeclContext()->getASTContext().Idents.get(
+ GuardName);
+ IdentifierInfoInitialized = true;
+}
+ }
+
+public:
+ RAIIMutexDescriptor(StringRef GuardName) : GuardName(GuardName) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+initIdentifierInfo(Call);
+const auto *Ctor = dyn_cast(&Call);
+if (!Ctor)
+ return false;
+auto *IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier();
+return IdentifierInfo == Guard;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+initIdentifierInfo(Call);
+const auto *Dtor = dyn_cast(&Call);
+if (!Dtor)
+ return false;
+auto *IdentifierInfo =
+cast(Dtor->getDecl()->getParent())->getIdentifier();
+return IdentifierInfo == Guard;
+ }
gamesh411 wrote:
Fixed.
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
@@ -70,73 +202,121 @@ class BlockInCriticalSectionChecker : public
Checker {
} // end anonymous namespace
-REGISTER_TRAIT_WITH_PROGRAMSTATE(MutexCounter, unsigned)
+REGISTER_LIST_WITH_PROGRAMSTATE(ActiveCritSections, CritSectionMarker)
-void BlockInCriticalSectionChecker::initIdentifierInfo(ASTContext &Ctx) const {
- if (!IdentifierInfoInitialized) {
-/* In case of checking C code, or when the corresponding headers are not
- * included, we might end up query the identifier table every time when
this
- * function is called instead of early returning it. To avoid this, a bool
- * variable (IdentifierInfoInitialized) is used and the function will be
run
- * only once. */
-IILockGuard = &Ctx.Idents.get(ClassLockGuard);
-IIUniqueLock = &Ctx.Idents.get(ClassUniqueLock);
-IdentifierInfoInitialized = true;
- }
-}
+namespace std {
+// Iterator traits for ImmutableList data structure
+// that enable the use of STL algorithms.
+// TODO: Move these to llvm::ImmutableList when overhauling immutable data
+// structures for proper iterator concept support.
+template <>
+struct iterator_traits<
+typename llvm::ImmutableList::iterator> {
+ using iterator_category = std::forward_iterator_tag;
+ using value_type = CritSectionMarker;
+ using difference_type = std::ptrdiff_t;
+ using reference = CritSectionMarker &;
+ using pointer = CritSectionMarker *;
+};
+} // namespace std
-bool BlockInCriticalSectionChecker::isBlockingFunction(const CallEvent &Call)
const {
- return matchesAny(Call, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn);
+std::optional
+BlockInCriticalSectionChecker::checkLock(const CallEvent &Call,
+ CheckerContext &C) const {
+ const auto *LockDescriptor =
+ llvm::find_if(MutexDescriptors, [&Call](auto &&LockFn) {
+return std::visit(
+[&Call](auto &&Descriptor) { return Descriptor.matchesLock(Call);
},
+LockFn);
+ });
+ if (LockDescriptor != MutexDescriptors.end())
+return *LockDescriptor;
+ return std::nullopt;
}
-bool BlockInCriticalSectionChecker::isLockFunction(const CallEvent &Call)
const {
- if (const auto *Ctor = dyn_cast(&Call)) {
-auto IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier();
-if (IdentifierInfo == IILockGuard || IdentifierInfo == IIUniqueLock)
- return true;
- }
+void BlockInCriticalSectionChecker::handleLock(
+const MutexDescriptor &LockDescriptor, const CallEvent &Call,
+CheckerContext &C) const {
+ const auto *MutexRegion = std::visit(
+ [&Call](auto &&Descriptor) { return Descriptor.getLockRegion(Call); },
+ LockDescriptor);
+ if (!MutexRegion)
+return;
+
+ const auto MarkToAdd = CritSectionMarker{Call.getOriginExpr(), MutexRegion};
+ ProgramStateRef StateWithLockEvent =
+ C.getState()->add(MarkToAdd);
+ C.addTransition(StateWithLockEvent, createCritSectionNote(MarkToAdd, C));
+}
- return matchesAny(Call, LockFn, PthreadLockFn, PthreadTryLockFn, MtxLock,
-MtxTimedLock, MtxTryLock);
+std::optional
+BlockInCriticalSectionChecker::checkUnlock(const CallEvent &Call,
+ CheckerContext &C) const {
+ const auto *UnlockDescriptor =
+ llvm::find_if(MutexDescriptors, [&Call](auto &&UnlockFn) {
+return std::visit(
+[&Call](auto &&Descriptor) {
+ return Descriptor.matchesUnlock(Call);
+},
+UnlockFn);
+ });
+ if (UnlockDescriptor != MutexDescriptors.end())
+return *UnlockDescriptor;
+ return std::nullopt;
gamesh411 wrote:
I think I have fixed most of the code duplication, thanks for the suggestions 👍
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
https://github.com/gamesh411 updated
https://github.com/llvm/llvm-project/pull/80029
From 346e2296869e750c7ec5bd75cf05f80a23b70569 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?=
Date: Tue, 30 Jan 2024 11:33:30 +0100
Subject: [PATCH 1/2] [clang][analyzer] Improve BlockInCriticalSectionsChecker
with multi-section and recursive mutex support
* Add support for multiple, potentially overlapping critical sections:
The checker can now simultaneously handle several mutex's critical
sections without confusing them.
* Implement the handling of recursive mutexes:
By identifying the lock events, recursive mutexes are now supported.
A lock event is a pair of a lock expression and the SVal of the mutex
that it locks, so even multiple locks of the same mutex (and even by
the same expression) is now supported.
* Refine the note tags generated by the checker:
The note tags now correctly show just for mutexes those are
active at point of error, and multiple acqisitions of the same mutex
are also noted.
---
.../BlockInCriticalSectionChecker.cpp | 391 ++
.../Analysis/block-in-critical-section.cpp| 270 +---
2 files changed, 515 insertions(+), 146 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 66e080adb1382b..74ec4b73bd8b5f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -20,48 +20,180 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class FirstArgMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call);
+ }
+ bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call);
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
+
+class RAIIMutexDescriptor {
+ mutable const IdentifierInfo *Guard{};
+ mutable bool IdentifierInfoInitialized{};
+ mutable llvm::SmallString<32> GuardName{};
+
+ void initIdentifierInfo(const CallEvent &Call) const {
+if (!IdentifierInfoInitialized) {
+ // In case of checking C code, or when the corresponding headers are not
+ // included, we might end up query the identifier table every time when
+ // this function is called instead of early returning it. To avoid this,
a
+ // bool variable (IdentifierInfoInitialized) is used and the function
will
+ // be run only once.
+ Guard = &Call.getCalleeAnalysisDeclContext()->getASTContext().Idents.get(
+ GuardName);
+ IdentifierInfoInitialized = true;
+}
+ }
+
+pu
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
@@ -20,48 +20,180 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class FirstArgMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call);
+ }
+ bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call);
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
+
+class RAIIMutexDescriptor {
+ mutable const IdentifierInfo *Guard{};
+ mutable bool IdentifierInfoInitialized{};
+ mutable llvm::SmallString<32> GuardName{};
+
+ void initIdentifierInfo(const CallEvent &Call) const {
+if (!IdentifierInfoInitialized) {
+ // In case of checking C code, or when the corresponding headers are not
+ // included, we might end up query the identifier table every time when
+ // this function is called instead of early returning it. To avoid this,
a
+ // bool variable (IdentifierInfoInitialized) is used and the function
will
+ // be run only once.
+ Guard = &Call.getCalleeAnalysisDeclContext()->getASTContext().Idents.get(
+ GuardName);
+ IdentifierInfoInitialized = true;
+}
+ }
+
+public:
+ RAIIMutexDescriptor(StringRef GuardName) : GuardName(GuardName) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+initIdentifierInfo(Call);
+const auto *Ctor = dyn_cast(&Call);
+if (!Ctor)
+ return false;
+auto *IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier();
+return IdentifierInfo == Guard;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+initIdentifierInfo(Call);
+const auto *Dtor = dyn_cast(&Call);
+if (!Dtor)
+ return false;
+auto *IdentifierInfo =
+cast(Dtor->getDecl()->getParent())->getIdentifier();
gamesh411 wrote:
The constructor and destructor cases are not symmetric; the getParent() for
CXXConstructorDecl uses the cast internally, while the getParent() only gives
DeclContext.
So, for the constructor, we have the following type chain:
`CXXConstructorCall` ---[ `getDecl()` ]---> `CXXConstructorDecl` ---[
`getParent()` ]---> `CXXRecordDecl`
and for the destructor we have the following:
`CXXDescructorCall` ---[ `getDecl()` ]---> `FunctionDecl` ---[ `getParent()`
]--> `DeclContext` (and this has to be cast to CXXRecordDecl)
The API is just not symmetric enough.
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
@@ -20,48 +20,180 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class FirstArgMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call);
+ }
+ bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call);
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
+
+class RAIIMutexDescriptor {
+ mutable const IdentifierInfo *Guard{};
+ mutable bool IdentifierInfoInitialized{};
+ mutable llvm::SmallString<32> GuardName{};
+
+ void initIdentifierInfo(const CallEvent &Call) const {
+if (!IdentifierInfoInitialized) {
+ // In case of checking C code, or when the corresponding headers are not
+ // included, we might end up query the identifier table every time when
+ // this function is called instead of early returning it. To avoid this,
a
+ // bool variable (IdentifierInfoInitialized) is used and the function
will
+ // be run only once.
+ Guard = &Call.getCalleeAnalysisDeclContext()->getASTContext().Idents.get(
+ GuardName);
+ IdentifierInfoInitialized = true;
+}
+ }
+
+public:
+ RAIIMutexDescriptor(StringRef GuardName) : GuardName(GuardName) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+initIdentifierInfo(Call);
+const auto *Ctor = dyn_cast(&Call);
+if (!Ctor)
+ return false;
+auto *IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier();
+return IdentifierInfo == Guard;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+initIdentifierInfo(Call);
+const auto *Dtor = dyn_cast(&Call);
+if (!Dtor)
+ return false;
+auto *IdentifierInfo =
+cast(Dtor->getDecl()->getParent())->getIdentifier();
NagyDonat wrote:
Why do you need a cast to `CXXRecordDecl` here if it isn't needed for the
constructor?
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
@@ -70,73 +202,121 @@ class BlockInCriticalSectionChecker : public
Checker {
} // end anonymous namespace
-REGISTER_TRAIT_WITH_PROGRAMSTATE(MutexCounter, unsigned)
+REGISTER_LIST_WITH_PROGRAMSTATE(ActiveCritSections, CritSectionMarker)
-void BlockInCriticalSectionChecker::initIdentifierInfo(ASTContext &Ctx) const {
- if (!IdentifierInfoInitialized) {
-/* In case of checking C code, or when the corresponding headers are not
- * included, we might end up query the identifier table every time when
this
- * function is called instead of early returning it. To avoid this, a bool
- * variable (IdentifierInfoInitialized) is used and the function will be
run
- * only once. */
-IILockGuard = &Ctx.Idents.get(ClassLockGuard);
-IIUniqueLock = &Ctx.Idents.get(ClassUniqueLock);
-IdentifierInfoInitialized = true;
- }
-}
+namespace std {
+// Iterator traits for ImmutableList data structure
+// that enable the use of STL algorithms.
+// TODO: Move these to llvm::ImmutableList when overhauling immutable data
+// structures for proper iterator concept support.
+template <>
+struct iterator_traits<
+typename llvm::ImmutableList::iterator> {
+ using iterator_category = std::forward_iterator_tag;
+ using value_type = CritSectionMarker;
+ using difference_type = std::ptrdiff_t;
+ using reference = CritSectionMarker &;
+ using pointer = CritSectionMarker *;
+};
+} // namespace std
-bool BlockInCriticalSectionChecker::isBlockingFunction(const CallEvent &Call)
const {
- return matchesAny(Call, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn);
+std::optional
+BlockInCriticalSectionChecker::checkLock(const CallEvent &Call,
+ CheckerContext &C) const {
+ const auto *LockDescriptor =
+ llvm::find_if(MutexDescriptors, [&Call](auto &&LockFn) {
+return std::visit(
+[&Call](auto &&Descriptor) { return Descriptor.matchesLock(Call);
},
+LockFn);
+ });
+ if (LockDescriptor != MutexDescriptors.end())
+return *LockDescriptor;
+ return std::nullopt;
}
-bool BlockInCriticalSectionChecker::isLockFunction(const CallEvent &Call)
const {
- if (const auto *Ctor = dyn_cast(&Call)) {
-auto IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier();
-if (IdentifierInfo == IILockGuard || IdentifierInfo == IIUniqueLock)
- return true;
- }
+void BlockInCriticalSectionChecker::handleLock(
+const MutexDescriptor &LockDescriptor, const CallEvent &Call,
+CheckerContext &C) const {
+ const auto *MutexRegion = std::visit(
+ [&Call](auto &&Descriptor) { return Descriptor.getLockRegion(Call); },
+ LockDescriptor);
+ if (!MutexRegion)
+return;
+
+ const auto MarkToAdd = CritSectionMarker{Call.getOriginExpr(), MutexRegion};
+ ProgramStateRef StateWithLockEvent =
+ C.getState()->add(MarkToAdd);
+ C.addTransition(StateWithLockEvent, createCritSectionNote(MarkToAdd, C));
+}
- return matchesAny(Call, LockFn, PthreadLockFn, PthreadTryLockFn, MtxLock,
-MtxTimedLock, MtxTryLock);
+std::optional
+BlockInCriticalSectionChecker::checkUnlock(const CallEvent &Call,
+ CheckerContext &C) const {
+ const auto *UnlockDescriptor =
+ llvm::find_if(MutexDescriptors, [&Call](auto &&UnlockFn) {
+return std::visit(
+[&Call](auto &&Descriptor) {
+ return Descriptor.matchesUnlock(Call);
+},
+UnlockFn);
+ });
+ if (UnlockDescriptor != MutexDescriptors.end())
+return *UnlockDescriptor;
+ return std::nullopt;
NagyDonat wrote:
This is an almost exact duplicate of `checkLock()`. It would be better to unify
them into a single function (e.g. `checkLockUnlock` that takes a boolean
argument `IsLock` (and then also unify `matchesLock`/`matchesUnlock` into a
single function that takes a boolean).
I know that boolean arguments can be a code smell, but in this particular case
it would be a much better solution than this code duplication.
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
@@ -20,48 +20,180 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class FirstArgMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call);
+ }
+ bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call);
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
+
+class RAIIMutexDescriptor {
+ mutable const IdentifierInfo *Guard{};
+ mutable bool IdentifierInfoInitialized{};
+ mutable llvm::SmallString<32> GuardName{};
+
+ void initIdentifierInfo(const CallEvent &Call) const {
+if (!IdentifierInfoInitialized) {
+ // In case of checking C code, or when the corresponding headers are not
+ // included, we might end up query the identifier table every time when
+ // this function is called instead of early returning it. To avoid this,
a
+ // bool variable (IdentifierInfoInitialized) is used and the function
will
+ // be run only once.
+ Guard = &Call.getCalleeAnalysisDeclContext()->getASTContext().Idents.get(
+ GuardName);
+ IdentifierInfoInitialized = true;
+}
+ }
+
+public:
+ RAIIMutexDescriptor(StringRef GuardName) : GuardName(GuardName) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+initIdentifierInfo(Call);
+const auto *Ctor = dyn_cast(&Call);
+if (!Ctor)
+ return false;
+auto *IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier();
+return IdentifierInfo == Guard;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+initIdentifierInfo(Call);
+const auto *Dtor = dyn_cast(&Call);
+if (!Dtor)
+ return false;
+auto *IdentifierInfo =
+cast(Dtor->getDecl()->getParent())->getIdentifier();
+return IdentifierInfo == Guard;
+ }
NagyDonat wrote:
The methods `matchesLock()` and `matchesUnlock()` are a bit complex and almost
identical, consider implementing them as calling
`matchesImpl(Call)` and
`matchesImpl(Call)` with a template function that contains
the shared logic.
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
@@ -20,48 +20,180 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
+
+#include
+#include
+#include
using namespace clang;
using namespace ento;
namespace {
+
+struct CritSectionMarker {
+ const Expr *LockExpr{};
+ const MemRegion *LockReg{};
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.Add(LockExpr);
+ID.Add(LockReg);
+ }
+
+ [[nodiscard]] constexpr bool
+ operator==(const CritSectionMarker &Other) const noexcept {
+return LockExpr == Other.LockExpr && LockReg == Other.LockReg;
+ }
+ [[nodiscard]] constexpr bool
+ operator!=(const CritSectionMarker &Other) const noexcept {
+return !(*this == Other);
+ }
+};
+
+class FirstArgMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ FirstArgMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call) && Call.getNumArgs() > 0;
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return Call.getArgSVal(0).getAsRegion();
+ }
+};
+
+class MemberMutexDescriptor {
+ CallDescription LockFn;
+ CallDescription UnlockFn;
+
+public:
+ MemberMutexDescriptor(CallDescription &&LockFn, CallDescription &&UnlockFn)
+ : LockFn(std::move(LockFn)), UnlockFn(std::move(UnlockFn)) {}
+ [[nodiscard]] bool matchesLock(const CallEvent &Call) const {
+return LockFn.matches(Call);
+ }
+ bool matchesUnlock(const CallEvent &Call) const {
+return UnlockFn.matches(Call);
+ }
+ [[nodiscard]] const MemRegion *getLockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+ [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const {
+return cast(Call).getCXXThisVal().getAsRegion();
+ }
+};
NagyDonat wrote:
Would it be possible to reduce the code duplication between these by
introducing a common ancestor class?
https://github.com/llvm/llvm-project/pull/80029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
https://github.com/NagyDonat commented: Seems to be a good change overall and I like that you implemented this with some nice helper classes. However, there is some code duplication that could be reduced (see inline comments). Also, I feel that (especially after these extensions) this checker duplicates some logic that also appears in `PthreadLockChecker.cpp`. I didn't check the exact details but that checker is also modeling the locking and unlocking calls in order to report bad locking/unlocking order. In theory it would be to ensure that we don't develop and maintain two similar but different solutions that both track lock handling code -- but PthreadLockChecker is 700 lines long and I understand that it would be difficult to unify it with this checker. https://github.com/llvm/llvm-project/pull/80029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/80029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
gamesh411 wrote: As keeping the trivial change that this PR was before, and thanks to the fact that the checker needed a major overhaul, I have increased the scope of this change to support the use cases mentioned by @balazske. That is why I am re-requesting a review. https://github.com/llvm/llvm-project/pull/80029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve BlockInCriticalSectionsChecker (PR #80029)
https://github.com/gamesh411 edited https://github.com/llvm/llvm-project/pull/80029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
