[analyzer] PthreadLock: Fix return values of XNU lock functions.

[NoQ]

Summary

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.

[zaks.anna]

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

271 ↗

(On Diff #115029)

This comment is repeated several times...

test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h

29 ↗

(On Diff #115029)

Should there be a test added that uses the function?

[NoQ]

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

271 ↗

(On Diff #115029)

Because it is relevant in both places. Should i rephrase with "also"/"as well"/"here too"?

test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h

29 ↗

(On Diff #115029)

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.

[a.sidorin]

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.

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

282 ↗

(On Diff #115029)

TODO?

[NoQ]

Hey wb! Get well :)

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

282 ↗

(On Diff #115029)

That'd make a terrible project of choice for people grepping for TODOs and FIXMEs, so I guess rather not.

[a.sidorin]

Looks good to me.

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

271 ↗

(On Diff #115029)

Small rephrasing will be good because it will clearly state that it is not a copy-paste error :)

clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE,
   if (!lockR)
     return;
 
   ProgramStateRef state = C.getState();
   const SymbolRef *sym = state->get<DestroyRetVal>(lockR);
   if (sym)
     state = resolvePossiblyDestroyedMutex(state, lockR, sym);
 
-  SVal X = C.getSVal(CE);
-  if (X.isUnknownOrUndef())
-    return;
-
-  DefinedSVal retVal = X.castAs<DefinedSVal>();
-
   if (const LockState *LState = state->get<LockMap>(lockR)) {
     if (LState->isLocked()) {
       if (!BT_doublelock)
         BT_doublelock.reset(new BugType(this, "Double locking",
                                         "Lock checker"));
       ExplodedNode *N = C.generateErrorNode();
       if (!N)
         return;
       auto report = std::make_unique<PathSensitiveBugReport>(
           *BT_doublelock, "This lock has already been acquired", N);
       report->addRange(CE->getArg(0)->getSourceRange());
       C.emitReport(std::move(report));
       return;
     } else if (LState->isDestroyed()) {
       reportUseDestroyedBug(C, CE);
       return;
     }
   }
 
   ProgramStateRef lockSucc = state;
   if (isTryLock) {
     // Bifurcate the state, and allow a mode where the lock acquisition fails.
+    SVal RetVal = state->getSVal(CE, C.getLocationContext());
+    if (auto DefinedRetVal = RetVal.getAs<DefinedSVal>()) {
       ProgramStateRef lockFail;
       switch (semantics) {
       case PthreadSemantics:
-      std::tie(lockFail, lockSucc) = state->assume(retVal);
+        std::tie(lockFail, lockSucc) = state->assume(*DefinedRetVal);
         break;
       case XNUSemantics:
-      std::tie(lockSucc, lockFail) = state->assume(retVal);
+        std::tie(lockSucc, lockFail) = state->assume(*DefinedRetVal);
         break;
       default:
         llvm_unreachable("Unknown tryLock locking semantics");
       }
       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);
+    SVal RetVal = state->getSVal(CE, C.getLocationContext());
+    if (auto DefinedRetVal = RetVal.getAs<DefinedSVal>()) {
+      // 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");
     lockSucc = state;
   }
 
   // Record that the lock was acquired.
   lockSucc = lockSucc->add<LockSet>(lockR);

clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h

 
 typedef pthread_mutex_t lck_mtx_t;
 
 extern int pthread_mutex_lock(pthread_mutex_t *);
 extern int pthread_mutex_unlock(pthread_mutex_t *);
 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);