[clang][analyzer] Improve BlockInCriticalSectionsChecker #80029
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Endre Fülöp (gamesh411) Changeschecker On entering a critical section, a note tag is now placed along the bugpath. Full diff: https://github.com/llvm/llvm-project/pull/80029.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 66e080adb1382..2b9b56ecdd374 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -57,6 +57,8 @@ class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
const CallEvent &call,
CheckerContext &C) const;
+ const NoteTag *createCriticalSectionNote(CheckerContext &C) const;
+
public:
bool isBlockingFunction(const CallEvent &Call) const;
bool isLockFunction(const CallEvent &Call) const;
@@ -126,8 +128,9 @@ void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call,
State = State->set<MutexCounter>(--mutexCount);
C.addTransition(State);
} else if (isLockFunction(Call)) {
+ const NoteTag *Note = createCriticalSectionNote(C);
State = State->set<MutexCounter>(++mutexCount);
- C.addTransition(State);
+ C.addTransition(State, Note);
} else if (mutexCount > 0) {
SymbolRef BlockDesc = Call.getReturnValue().getAsSymbol();
reportBlockInCritSection(BlockDesc, Call, C);
@@ -151,10 +154,22 @@ void BlockInCriticalSectionChecker::reportBlockInCritSection(
C.emitReport(std::move(R));
}
+const NoteTag *BlockInCriticalSectionChecker::createCriticalSectionNote(
+ CheckerContext &C) const {
+ const BugType *BT = &this->BlockInCritSectionBugType;
+ return C.getNoteTag(
+ [BT](PathSensitiveBugReport &BR, llvm::raw_ostream &OS) {
+ if(&BR.getBugType() != BT)
+ return;
+ OS << "Entering critical section here";
+ });
+}
+
void ento::registerBlockInCriticalSectionChecker(CheckerManager &mgr) {
mgr.registerChecker<BlockInCriticalSectionChecker>();
}
-bool ento::shouldRegisterBlockInCriticalSectionChecker(const CheckerManager &mgr) {
+bool ento::shouldRegisterBlockInCriticalSectionChecker(
+ const CheckerManager &mgr) {
return true;
}
diff --git a/clang/test/Analysis/block-in-critical-section.cpp b/clang/test/Analysis/block-in-critical-section.cpp
index fcf6188fc033e..93d46c741e16f 100644
--- a/clang/test/Analysis/block-in-critical-section.cpp
+++ b/clang/test/Analysis/block-in-critical-section.cpp
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.BlockInCriticalSection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=alpha.unix.BlockInCriticalSection \
+// RUN: -std=c++11 \
+// RUN: -analyzer-output text \
+// RUN: -verify %s
void sleep(int x) {}
@@ -21,7 +25,7 @@ template<typename T>
struct not_real_lock {
not_real_lock<T>(std::mutex) {}
};
-}
+} // namespace std
void getc() {}
void fgets() {}
@@ -39,81 +43,115 @@ void mtx_unlock() {}
void testBlockInCriticalSectionWithStdMutex() {
std::mutex m;
- m.lock();
+ m.lock(); // expected-note 5{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
m.unlock();
}
void testBlockInCriticalSectionWithPthreadMutex() {
- pthread_mutex_lock();
+ pthread_mutex_lock(); // expected-note 10{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
pthread_mutex_unlock();
- pthread_mutex_trylock();
+ pthread_mutex_trylock(); // expected-note 5{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
pthread_mutex_unlock();
}
void testBlockInCriticalSectionC11Locks() {
- mtx_lock();
+ mtx_lock(); // expected-note 15{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
mtx_unlock();
- mtx_timedlock();
+ mtx_timedlock(); // expected-note 10{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
mtx_unlock();
- mtx_trylock();
+ mtx_trylock(); // expected-note 5{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
mtx_unlock();
}
void testBlockInCriticalSectionWithNestedMutexes() {
std::mutex m, n, k;
- m.lock();
- n.lock();
- k.lock();
+ m.lock(); // expected-note 3{{Entering critical section here}}
+ n.lock(); // expected-note 3{{Entering critical section here}}
+ k.lock(); // expected-note 3{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
k.unlock();
sleep(5); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
n.unlock();
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
m.unlock();
sleep(3); // no-warning
}
void f() {
sleep(1000); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
}
void testBlockInCriticalSectionInterProcedural() {
std::mutex m;
- m.lock();
- f();
+ m.lock(); // expected-note {{Entering critical section here}}
+ f(); // expected-note {{Calling 'f'}}
m.unlock();
}
@@ -121,8 +159,9 @@ void testBlockInCriticalSectionUnexpectedUnlock() {
std::mutex m;
m.unlock();
sleep(1); // no-warning
- m.lock();
+ m.lock(); // expected-note {{Entering critical section here}}
sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
}
void testBlockInCriticalSectionLockGuard() {
@@ -130,12 +169,13 @@ void testBlockInCriticalSectionLockGuard() {
std::not_real_lock<std::mutex> not_real_lock(g_mutex);
sleep(1); // no-warning
- std::lock_guard<std::mutex> lock(g_mutex);
+ std::lock_guard<std::mutex> lock(g_mutex); // expected-note {{Entering critical section here}}
sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
}
void testBlockInCriticalSectionLockGuardNested() {
- testBlockInCriticalSectionLockGuard();
+ testBlockInCriticalSectionLockGuard(); // expected-note {{Calling 'testBlockInCriticalSectionLockGuard'}}
sleep(1); // no-warning
}
@@ -144,11 +184,12 @@ void testBlockInCriticalSectionUniqueLock() {
std::not_real_lock<std::mutex> not_real_lock(g_mutex);
sleep(1); // no-warning
- std::unique_lock<std::mutex> lock(g_mutex);
+ std::unique_lock<std::mutex> lock(g_mutex); // expected-note {{Entering critical section here}}
sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
}
void testBlockInCriticalSectionUniqueLockNested() {
- testBlockInCriticalSectionUniqueLock();
+ testBlockInCriticalSectionUniqueLock(); // expected-note {{Calling 'testBlockInCriticalSectionUniqueLock'}}
sleep(1); // no-warning
}
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Endre Fülöp (gamesh411) Changeschecker On entering a critical section, a note tag is now placed along the bugpath. Full diff: https://github.com/llvm/llvm-project/pull/80029.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 66e080adb1382..2b9b56ecdd374 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -57,6 +57,8 @@ class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
const CallEvent &call,
CheckerContext &C) const;
+ const NoteTag *createCriticalSectionNote(CheckerContext &C) const;
+
public:
bool isBlockingFunction(const CallEvent &Call) const;
bool isLockFunction(const CallEvent &Call) const;
@@ -126,8 +128,9 @@ void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call,
State = State->set<MutexCounter>(--mutexCount);
C.addTransition(State);
} else if (isLockFunction(Call)) {
+ const NoteTag *Note = createCriticalSectionNote(C);
State = State->set<MutexCounter>(++mutexCount);
- C.addTransition(State);
+ C.addTransition(State, Note);
} else if (mutexCount > 0) {
SymbolRef BlockDesc = Call.getReturnValue().getAsSymbol();
reportBlockInCritSection(BlockDesc, Call, C);
@@ -151,10 +154,22 @@ void BlockInCriticalSectionChecker::reportBlockInCritSection(
C.emitReport(std::move(R));
}
+const NoteTag *BlockInCriticalSectionChecker::createCriticalSectionNote(
+ CheckerContext &C) const {
+ const BugType *BT = &this->BlockInCritSectionBugType;
+ return C.getNoteTag(
+ [BT](PathSensitiveBugReport &BR, llvm::raw_ostream &OS) {
+ if(&BR.getBugType() != BT)
+ return;
+ OS << "Entering critical section here";
+ });
+}
+
void ento::registerBlockInCriticalSectionChecker(CheckerManager &mgr) {
mgr.registerChecker<BlockInCriticalSectionChecker>();
}
-bool ento::shouldRegisterBlockInCriticalSectionChecker(const CheckerManager &mgr) {
+bool ento::shouldRegisterBlockInCriticalSectionChecker(
+ const CheckerManager &mgr) {
return true;
}
diff --git a/clang/test/Analysis/block-in-critical-section.cpp b/clang/test/Analysis/block-in-critical-section.cpp
index fcf6188fc033e..93d46c741e16f 100644
--- a/clang/test/Analysis/block-in-critical-section.cpp
+++ b/clang/test/Analysis/block-in-critical-section.cpp
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.BlockInCriticalSection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=alpha.unix.BlockInCriticalSection \
+// RUN: -std=c++11 \
+// RUN: -analyzer-output text \
+// RUN: -verify %s
void sleep(int x) {}
@@ -21,7 +25,7 @@ template<typename T>
struct not_real_lock {
not_real_lock<T>(std::mutex) {}
};
-}
+} // namespace std
void getc() {}
void fgets() {}
@@ -39,81 +43,115 @@ void mtx_unlock() {}
void testBlockInCriticalSectionWithStdMutex() {
std::mutex m;
- m.lock();
+ m.lock(); // expected-note 5{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
m.unlock();
}
void testBlockInCriticalSectionWithPthreadMutex() {
- pthread_mutex_lock();
+ pthread_mutex_lock(); // expected-note 10{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
pthread_mutex_unlock();
- pthread_mutex_trylock();
+ pthread_mutex_trylock(); // expected-note 5{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
pthread_mutex_unlock();
}
void testBlockInCriticalSectionC11Locks() {
- mtx_lock();
+ mtx_lock(); // expected-note 15{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
mtx_unlock();
- mtx_timedlock();
+ mtx_timedlock(); // expected-note 10{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
mtx_unlock();
- mtx_trylock();
+ mtx_trylock(); // expected-note 5{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
getc(); // expected-warning {{Call to blocking function 'getc' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'getc' inside of critical section}}
fgets(); // expected-warning {{Call to blocking function 'fgets' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'fgets' inside of critical section}}
read(); // expected-warning {{Call to blocking function 'read' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'read' inside of critical section}}
recv(); // expected-warning {{Call to blocking function 'recv' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'recv' inside of critical section}}
mtx_unlock();
}
void testBlockInCriticalSectionWithNestedMutexes() {
std::mutex m, n, k;
- m.lock();
- n.lock();
- k.lock();
+ m.lock(); // expected-note 3{{Entering critical section here}}
+ n.lock(); // expected-note 3{{Entering critical section here}}
+ k.lock(); // expected-note 3{{Entering critical section here}}
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
k.unlock();
sleep(5); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
n.unlock();
sleep(3); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
m.unlock();
sleep(3); // no-warning
}
void f() {
sleep(1000); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
}
void testBlockInCriticalSectionInterProcedural() {
std::mutex m;
- m.lock();
- f();
+ m.lock(); // expected-note {{Entering critical section here}}
+ f(); // expected-note {{Calling 'f'}}
m.unlock();
}
@@ -121,8 +159,9 @@ void testBlockInCriticalSectionUnexpectedUnlock() {
std::mutex m;
m.unlock();
sleep(1); // no-warning
- m.lock();
+ m.lock(); // expected-note {{Entering critical section here}}
sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
}
void testBlockInCriticalSectionLockGuard() {
@@ -130,12 +169,13 @@ void testBlockInCriticalSectionLockGuard() {
std::not_real_lock<std::mutex> not_real_lock(g_mutex);
sleep(1); // no-warning
- std::lock_guard<std::mutex> lock(g_mutex);
+ std::lock_guard<std::mutex> lock(g_mutex); // expected-note {{Entering critical section here}}
sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
}
void testBlockInCriticalSectionLockGuardNested() {
- testBlockInCriticalSectionLockGuard();
+ testBlockInCriticalSectionLockGuard(); // expected-note {{Calling 'testBlockInCriticalSectionLockGuard'}}
sleep(1); // no-warning
}
@@ -144,11 +184,12 @@ void testBlockInCriticalSectionUniqueLock() {
std::not_real_lock<std::mutex> not_real_lock(g_mutex);
sleep(1); // no-warning
- std::unique_lock<std::mutex> lock(g_mutex);
+ std::unique_lock<std::mutex> lock(g_mutex); // expected-note {{Entering critical section here}}
sleep(1); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
}
void testBlockInCriticalSectionUniqueLockNested() {
- testBlockInCriticalSectionUniqueLock();
+ testBlockInCriticalSectionUniqueLock(); // expected-note {{Calling 'testBlockInCriticalSectionUniqueLock'}}
sleep(1); // no-warning
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
gamesh411
force-pushed
the
block-in-critical-notes
branch
from
54da4f5 to
f7875a7
Compare
|
I think we could get a note at the first |
balazske
reviewed
Feb 8, 2024
| CheckerContext &C) const; | ||
| const NoteTag * | ||
| createCriticalSectionNote(const CritSectionMarker &CriticalSectionBegin, | ||
| CheckerContext &C) const; |
There was a problem hiding this comment.
I do not know if it is sufficient to store the set of all found lock calls until the last unlock happens. Probably in the following situation it does not work:
std::mutex m, n, k;
m.lock();
n.lock();
n.unlock();
k.lock();
k.unlock();
sleep(1);
m.unlock();
It looks better to store all locked mutex objects somehow (not the lock calls), and remove one at unlock. Then the mutex counter is not needed.
There was a problem hiding this comment.
Possible improvements (but not necessary to move the checker out of alpha):
Handle recursive_mutex, lock_guard, unique_lock, scoped_lock objects. (It is recommended to use lock wrappers instead of plain lock and unlock functions.)
…ection 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.
gamesh411
force-pushed
the
block-in-critical-notes
branch
from
a39f9e4 to
346e229
Compare
gamesh411
changed the title
|
After some consideration I concluded that keeping the trivial change was pointless, and thanks to the fact that the checker needed a significant overhaul, I have increased the scope of this change to support the use cases mentioned by @balazske. |
NagyDonat
reviewed
Mar 11, 2024
There was a problem hiding this comment.
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.
Comment on lines
58
to
98
| 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<CXXMemberCall>(Call).getCXXThisVal().getAsRegion(); | ||
| } | ||
| [[nodiscard]] const MemRegion *getUnlockRegion(const CallEvent &Call) const { | ||
| return cast<CXXMemberCall>(Call).getCXXThisVal().getAsRegion(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Would it be possible to reduce the code duplication between these by introducing a common ancestor class?
There was a problem hiding this comment.
Introduced a common base class for these 2 classes.
| if (!Dtor) | ||
| return false; | ||
| auto *IdentifierInfo = | ||
| cast<CXXRecordDecl>(Dtor->getDecl()->getParent())->getIdentifier(); |
There was a problem hiding this comment.
Why do you need a cast to CXXRecordDecl here if it isn't needed for the constructor?
There was a problem hiding this comment.
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.
Comment on lines
120
to
136
| [[nodiscard]] bool matchesLock(const CallEvent &Call) const { | ||
| initIdentifierInfo(Call); | ||
| const auto *Ctor = dyn_cast<CXXConstructorCall>(&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<CXXDestructorCall>(&Call); | ||
| if (!Dtor) | ||
| return false; | ||
| auto *IdentifierInfo = | ||
| cast<CXXRecordDecl>(Dtor->getDecl()->getParent())->getIdentifier(); | ||
| return IdentifierInfo == Guard; | ||
| } |
There was a problem hiding this comment.
The methods matchesLock() and matchesUnlock() are a bit complex and almost identical, consider implementing them as calling matchesImpl<CXXConstructorCall>(Call) and matchesImpl<CXXDestructorCall>(Call) with a template function that contains the shared logic.
Comment on lines
252
to
265
| std::optional<MutexDescriptor> | ||
| 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think I have fixed most of the code duplication, thanks for the suggestions 👍
MSVC has an implementation where the return value of std::find_if is a proper iterator not a pointer In order to support that, we use type deduction now
NagyDonat
approved these changes
Mar 18, 2024
Comment on lines
+67
to
+69
| if (IsLock) { | ||
| return LockFn.matches(Call); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (IsLock) { | |
| return LockFn.matches(Call); | |
| } | |
| 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);
Comment on lines
+125
to
+127
| if (IsLock) { | ||
| return matchesImpl<CXXConstructorCall>(Call); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (IsLock) { | |
| return matchesImpl<CXXConstructorCall>(Call); | |
| } | |
| if (IsLock) | |
| return matchesImpl<CXXConstructorCall>(Call); |
Bikeshedding as above.
Comment on lines
+134
to
+136
| if (std::optional<SVal> Object = Call.getReturnValueUnderConstruction()) { | ||
| LockRegion = Object->getAsRegion(); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (std::optional<SVal> Object = Call.getReturnValueUnderConstruction()) { | |
| LockRegion = Object->getAsRegion(); | |
| } | |
| if (std::optional<SVal> Object = Call.getReturnValueUnderConstruction()) | |
| LockRegion = Object->getAsRegion(); |
Yet another brace tweak...
|
It looks like that this change causes crashes on many projects (curl, vim, postgres, others) in |
chencha3
pushed a commit
to chencha3/llvm-project
that referenced
this pull request
Mar 23, 2024
* 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 that are active at the point of error, and multiple acquisitions of the same mutex are also noted.
gamesh411
added a commit
that referenced
this pull request
Jun 3, 2024
After recent improvements (#80029) and testing on open-source projects, the checker is ready to move out of the alpha package.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On entering a critical section, a note tag is now placed along the bugpath.