[PATCH] D37806: [analyzer] PthreadLock: Fix return values of XNU lock functions.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG80fd37f9d66e: [analyzer] PthreadLock: Fix return value
modeling for XNU lock functions. (authored by dergachev.a).
Herald added subscribers: Charusso, dkrupp, donat.nagy, jfb, Szelethus,
mikhail.ramalho, rnkovacs, szepet, baloghadamsoftware.
Herald added a project: clang.
Changed prior to commit:
https://reviews.llvm.org/D37806?vs=115029&id=240197#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D37806/new/
https://reviews.llvm.org/D37806
Files:
clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
Index: clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
+++ clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
@@ -22,7 +22,9 @@
extern int pthread_mutex_trylock(pthread_mutex_t *);
extern int pthread_mutex_destroy(pthread_mutex_t *);
extern int pthread_mutex_init(pthread_mutex_t *mutex, const
pthread_mutexattr_t *mutexattr);
-extern int lck_mtx_lock(lck_mtx_t *);
-extern int lck_mtx_unlock(lck_mtx_t *);
-extern int lck_mtx_try_lock(lck_mtx_t *);
+
+typedef int boolean_t;
+extern void lck_mtx_lock(lck_mtx_t *);
+extern void lck_mtx_unlock(lck_mtx_t *);
+extern boolean_t lck_mtx_try_lock(lck_mtx_t *);
extern void lck_mtx_destroy(lck_mtx_t *lck, lck_grp_t *grp);
Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -226,12 +226,6 @@
if (sym)
state = resolvePossiblyDestroyedMutex(state, lockR, sym);
- SVal X = C.getSVal(CE);
- if (X.isUnknownOrUndef())
-return;
-
- DefinedSVal retVal = X.castAs();
-
if (const LockState *LState = state->get(lockR)) {
if (LState->isLocked()) {
if (!BT_doublelock)
@@ -254,25 +248,35 @@
ProgramStateRef lockSucc = state;
if (isTryLock) {
// Bifurcate the state, and allow a mode where the lock acquisition fails.
-ProgramStateRef lockFail;
-switch (semantics) {
-case PthreadSemantics:
- std::tie(lockFail, lockSucc) = state->assume(retVal);
- break;
-case XNUSemantics:
- std::tie(lockSucc, lockFail) = state->assume(retVal);
- break;
-default:
- llvm_unreachable("Unknown tryLock locking semantics");
+SVal RetVal = state->getSVal(CE, C.getLocationContext());
+if (auto DefinedRetVal = RetVal.getAs()) {
+ 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);
}
-assert(lockFail && lockSucc);
-C.addTransition(lockFail);
-
+// We might want to handle the case when the mutex lock function was
inlined
+// and returned an Unknown or Undefined value.
} else if (semantics == PthreadSemantics) {
// Assume that the return value was 0.
-lockSucc = state->assume(retVal, false);
-assert(lockSucc);
-
+SVal RetVal = state->getSVal(CE, C.getLocationContext());
+if (auto DefinedRetVal = RetVal.getAs()) {
+ // FIXME: If the lock function was inlined and returned true,
+ // we need to behave sanely - at least generate sink.
+ lockSucc = state->assume(*DefinedRetVal, false);
+ assert(lockSucc);
+}
+// We might want to handle the case when the mutex lock function was
inlined
+// and returned an Unknown or Undefined value.
} else {
// XNU locking semantics return void on non-try locks
assert((semantics == XNUSemantics) && "Unknown locking semantics");
Index: clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
+++ clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
@@ -22,7 +22,9 @@
extern int pthread_mutex_trylock(pthread_mutex_t *);
extern int pthread_mutex_destroy(pthread_mutex_t *);
extern int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t *mutexattr);
-extern int lck_mtx_lock(lck_mtx_t *);
-extern int lck_mtx_unlock(lck_mtx_t *);
-extern int lck_mtx_try_lock(lck_mtx_t *);
+
+typedef int boolean_t;
+extern void lck_mtx_lock(lck_mtx_t *);
+extern void lck_mtx_unlock(lck_mtx_t *);
+extern boolean_t lck_mtx_try_lock(lck_mtx_t *);
exte
[PATCH] D37806: [analyzer] PthreadLock: Fix return values of XNU lock functions.
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Looks good to me. Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:271 } -assert(lockFail && lockSucc); -C.addTransition(lockFail); - +// We might want to handle the case when the mutex lock function was inlined +// and returned an Unknown or Undefined value. NoQ wrote: > zaks.anna wrote: > > This comment is repeated several times... > Because it is relevant in both places. Should i rephrase with "also"/"as > well"/"here too"? Small rephrasing will be good because it will clearly state that it is not a copy-paste error :) https://reviews.llvm.org/D37806 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37806: [analyzer] PthreadLock: Fix return values of XNU lock functions.
NoQ added a comment. Hey wb! Get well :) Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:282 +} +// We might want to handle the case when the mutex lock function was inlined +// and returned an Unknown or Undefined value. a.sidorin wrote: > TODO? That'd make a terrible project of choice for people grepping for TODOs and FIXMEs, so I guess rather not. https://reviews.llvm.org/D37806 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37806: [analyzer] PthreadLock: Fix return values of XNU lock functions.
a.sidorin added a comment. Hi Artem, Sorry for long delay for reviews. Unfortunately, hospital is a bad place to do a code review and broken hand is a bad review assistant. This patch looks good to me, I have just a minor comment nit. Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:282 +} +// We might want to handle the case when the mutex lock function was inlined +// and returned an Unknown or Undefined value. TODO? https://reviews.llvm.org/D37806 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37806: [analyzer] PthreadLock: Fix return values of XNU lock functions.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:271 } -assert(lockFail && lockSucc); -C.addTransition(lockFail); - +// We might want to handle the case when the mutex lock function was inlined +// and returned an Unknown or Undefined value. zaks.anna wrote: > This comment is repeated several times... Because it is relevant in both places. Should i rephrase with "also"/"as well"/"here too"? Comment at: test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h:29 +extern void lck_mtx_unlock(lck_mtx_t *); +extern boolean_t lck_mtx_try_lock(lck_mtx_t *); extern void lck_mtx_destroy(lck_mtx_t *lck, lck_grp_t *grp); zaks.anna wrote: > Should there be a test added that uses the function? The tests are already there for all three functions. I just fixed their prototype to match the real-world prototype, and then adjusted the code accordingly, so that the existing tests kept passing. https://reviews.llvm.org/D37806 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37806: [analyzer] PthreadLock: Fix return values of XNU lock functions.
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:271 } -assert(lockFail && lockSucc); -C.addTransition(lockFail); - +// We might want to handle the case when the mutex lock function was inlined +// and returned an Unknown or Undefined value. This comment is repeated several times... Comment at: test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h:29 +extern void lck_mtx_unlock(lck_mtx_t *); +extern boolean_t lck_mtx_try_lock(lck_mtx_t *); extern void lck_mtx_destroy(lck_mtx_t *lck, lck_grp_t *grp); Should there be a test added that uses the function? https://reviews.llvm.org/D37806 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37806: [analyzer] PthreadLock: Fix return values of XNU lock functions.
NoQ created this revision.
`lck_mtx_lock()` returns `void`. The analyzer failed to model its effect
because he was surprised that the return value is `Unknown`. Prepare for the
aforementioned surprise and fix the tests accordingly.
https://reviews.llvm.org/D37806
Files:
lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
Index: test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
===
--- test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
+++ test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
@@ -22,7 +22,9 @@
extern int pthread_mutex_trylock(pthread_mutex_t *);
extern int pthread_mutex_destroy(pthread_mutex_t *);
extern int pthread_mutex_init(pthread_mutex_t *mutex, const
pthread_mutexattr_t *mutexattr);
-extern int lck_mtx_lock(lck_mtx_t *);
-extern int lck_mtx_unlock(lck_mtx_t *);
-extern int lck_mtx_try_lock(lck_mtx_t *);
+
+typedef int boolean_t;
+extern void lck_mtx_lock(lck_mtx_t *);
+extern void lck_mtx_unlock(lck_mtx_t *);
+extern boolean_t lck_mtx_try_lock(lck_mtx_t *);
extern void lck_mtx_destroy(lck_mtx_t *lck, lck_grp_t *grp);
Index: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -230,12 +230,6 @@
if (sym)
state = resolvePossiblyDestroyedMutex(state, lockR, sym);
- SVal X = state->getSVal(CE, C.getLocationContext());
- if (X.isUnknownOrUndef())
-return;
-
- DefinedSVal retVal = X.castAs();
-
if (const LockState *LState = state->get(lockR)) {
if (LState->isLocked()) {
if (!BT_doublelock)
@@ -258,25 +252,35 @@
ProgramStateRef lockSucc = state;
if (isTryLock) {
// Bifurcate the state, and allow a mode where the lock acquisition fails.
-ProgramStateRef lockFail;
-switch (semantics) {
-case PthreadSemantics:
- std::tie(lockFail, lockSucc) = state->assume(retVal);
- break;
-case XNUSemantics:
- std::tie(lockSucc, lockFail) = state->assume(retVal);
- break;
-default:
- llvm_unreachable("Unknown tryLock locking semantics");
+SVal RetVal = state->getSVal(CE, C.getLocationContext());
+if (auto DefinedRetVal = RetVal.getAs()) {
+ 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);
}
-assert(lockFail && lockSucc);
-C.addTransition(lockFail);
-
+// We might want to handle the case when the mutex lock function was
inlined
+// and returned an Unknown or Undefined value.
} else if (semantics == PthreadSemantics) {
// Assume that the return value was 0.
-lockSucc = state->assume(retVal, false);
-assert(lockSucc);
-
+SVal RetVal = state->getSVal(CE, C.getLocationContext());
+if (auto DefinedRetVal = RetVal.getAs()) {
+ // FIXME: If the lock function was inlined and returned true,
+ // we need to behave sanely - at least generate sink.
+ lockSucc = state->assume(*DefinedRetVal, false);
+ assert(lockSucc);
+}
+// We might want to handle the case when the mutex lock function was
inlined
+// and returned an Unknown or Undefined value.
} else {
// XNU locking semantics return void on non-try locks
assert((semantics == XNUSemantics) && "Unknown locking semantics");
Index: test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
===
--- test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
+++ test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
@@ -22,7 +22,9 @@
extern int pthread_mutex_trylock(pthread_mutex_t *);
extern int pthread_mutex_destroy(pthread_mutex_t *);
extern int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t *mutexattr);
-extern int lck_mtx_lock(lck_mtx_t *);
-extern int lck_mtx_unlock(lck_mtx_t *);
-extern int lck_mtx_try_lock(lck_mtx_t *);
+
+typedef int boolean_t;
+extern void lck_mtx_lock(lck_mtx_t *);
+extern void lck_mtx_unlock(lck_mtx_t *);
+extern boolean_t lck_mtx_try_lock(lck_mtx_t *);
extern void lck_mtx_destroy(lck_mtx_t *lck, lck_grp_t *grp);
Index: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -230,12 +230,6 @@
if (sym)
state = resolvePossiblyDest
