[clang][analyzer][NFC] Add test for a limitation of alpha.unix.Bloc…

[gamesh411]

…kInCriticalSection checker

Updated the documentation in checkers.rst to include an example of how trylock function is handled.
Added a new test for a scenario where pthread_mutex_trylock is used, demonstrating the current limitation.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Endre Fülöp (gamesh411)

Changes

…kInCriticalSection checker

Updated the documentation in checkers.rst to include an example of how trylock function is handled.
Added a new test for a scenario where pthread_mutex_trylock is used, demonstrating the current limitation.

---

Full diff: https://github.com/llvm/llvm-project/pull/93799.diff

2 Files Affected:

• (modified) clang/docs/analyzer/checkers.rst (+15)
• (modified) clang/test/Analysis/block-in-critical-section.cpp (+24-7)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 3a31708a1e9de..b677c5f3efa04 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -3148,6 +3148,21 @@ Applies to: ``lock, unlock, sleep, getc, fgets, read, recv, pthread_mutex_lock,`
    m.unlock();
  }
 
+**Limitations**
+* The ``trylock`` and ``timedlock`` versions of acquiring locks are currently assumed to always succeed.
+  This can lead to false positives.
+
+.. code-block:: c
+
+void trylock_example(pthread_mutex_t *m) {
+  if (pthread_mutex_trylock(m) == 0) { // assume trylock always succeeds
+    sleep(10); // warn: Call to blocking function 'sleep' inside of critical section
+    pthread_mutex_unlock(m);
+  } else {
+    sleep(10); // false positive: Incorrect warning about blocking function inside critical section.
+  }
+}
+
 .. _alpha-unix-Chroot:
 
 alpha.unix.Chroot (C)
diff --git a/clang/test/Analysis/block-in-critical-section.cpp b/clang/test/Analysis/block-in-critical-section.cpp
index 87c26b9f1b520..403b7a16726a2 100644
--- a/clang/test/Analysis/block-in-critical-section.cpp
+++ b/clang/test/Analysis/block-in-critical-section.cpp
@@ -36,15 +36,15 @@ ssize_t read(int fd, void *buf, size_t count);
 ssize_t recv(int sockfd, void *buf, size_t len, int flags);
 
 struct pthread_mutex_t;
-void pthread_mutex_lock(pthread_mutex_t *mutex);
-void pthread_mutex_trylock(pthread_mutex_t *mutex);
-void pthread_mutex_unlock(pthread_mutex_t *mutex);
+int pthread_mutex_lock(pthread_mutex_t *mutex);
+int pthread_mutex_trylock(pthread_mutex_t *mutex);
+int pthread_mutex_unlock(pthread_mutex_t *mutex);
 
 struct mtx_t;
-void mtx_lock(mtx_t *mutex);
-void mtx_timedlock(mtx_t *mutex);
-void mtx_trylock(mtx_t *mutex);
-void mtx_unlock(mtx_t *mutex);
+int mtx_lock(mtx_t *mutex);
+int mtx_timedlock(mtx_t *mutex);
+int mtx_trylock(mtx_t *mutex);
+int mtx_unlock(mtx_t *mutex);
 
 // global params for dummy function calls
 FILE *stream;
@@ -292,3 +292,20 @@ void testBlockInCriticalSectionUniqueLockNested() {
   testBlockInCriticalSectionUniqueLock(); // expected-note {{Calling 'testBlockInCriticalSectionUniqueLock'}}
   sleep(1); // no-warning
 }
+
+void testTrylockCurrentlyFalsePositive(pthread_mutex_t *m) {
+                                       // expected-note@+4 {{Assuming the condition is true}}
+                                       // expected-note@+3 {{Taking true branch}}
+                                       // expected-note@+2 {{Assuming the condition is false}}
+                                       // expected-note@+1 {{Taking false branch}}
+  if (pthread_mutex_trylock(m) == 0) { // expected-note 2 {{Entering critical section here}}
+                                       // FIXME: we are entering the critical section only in the true branch
+    sleep(10); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+               // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
+    pthread_mutex_unlock(m);
+  } else {
+    sleep(10); // expected-warning {{Call to blocking function 'sleep' inside of critical section}}
+               // expected-note@-1 {{Call to blocking function 'sleep' inside of critical section}}
+               // FIXME: this is a false positive, the lock was not acquired
+  }
+}

[NagyDonat]

It's good to document this, the commit LGTM. Are you planning to fix this soon?

[gamesh411]

It's good to document this, the commit LGTM. Are you planning to fix this soon?

I have been looking into the alpha.unix.PthreadLock checker. By reusing the logic there

llvm-project/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

Lines 454 to 472 in b86a9c5

	ProgramStateRef lockSucc = state;
	if (IsTryLock) {
	// Bifurcate the state, and allow a mode where the lock acquisition fails.
	SVal RetVal = Call.getReturnValue();
	if (auto DefinedRetVal = RetVal.getAs<DefinedSVal>()) {
	ProgramStateRef lockFail;
	switch (Semantics) {
	case PthreadSemantics:
	std::tie(lockFail, lockSucc) = state->assume(*DefinedRetVal);
	break;
	case XNUSemantics:
	std::tie(lockSucc, lockFail) = state->assume(*DefinedRetVal);
	break;
	default:
	llvm_unreachable("Unknown tryLock locking semantics");
	}
	assert(lockFail && lockSucc);
	C.addTransition(lockFail);
	}

that detects whether a lock has indeed been locked or not, we also get rid of this limitation "for free" (Aside from the fact that we have to unify the implementations because there is enough effective code duplication across these checkers as currently stands, but this is the way forward IMO, and I'm currently working on this).