[analyzer] PthreadLock: Refactor, use PostCall API. NFC.

[NoQ]

Summary

Use CallEvent and CallDescription everywhere. Unhardcode argument numbers in AcquireLock() etc. Have a list of supported functions in one place. Other misc cleanup. No functional change intended anywhere.

[NoQ]

Don't forget to check that the function is a global C function in post-call.

[zaks.anna]

How about committing the refactor of the code without test modifications. And committing changes to the test separately?

[NoQ]

Remove the changes in tests for now. I guess they'd need more cleanup anyway.

[a.sidorin]

I like this refactoring. I wrote some things that are not clear for me inline.

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

107

In the code below lock is always Call.getSVal(ArgNo). Should we remove it and put SVal acquisition into callees?

108

According to naming conventions , it should be Lock and IsTryLock. But I'm not sure that this corresponds to the file's code style so I do not insist.

188

break?

262

Could you add comments describing what arguments should do? I.e. `/* ArgNo= */ 0, /* Lock= */...? It's hard to remember what do these parameters correspond to.

[NoQ]

This patch has already landed as rG152bc7ffdcd8f62b2279803642f162610154cd2e. I forgot the magic word.

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
 struct LockState {
   enum Kind {
public:
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddInteger(K);
   }
 };
 
 class PthreadLockChecker
-    : public Checker<check::PostStmt<CallExpr>, check::DeadSymbols> {
+    : public Checker<check::PostCall, check::DeadSymbols> {
   mutable std::unique_ptr<BugType> BT_doublelock;
   mutable std::unique_ptr<BugType> BT_doubleunlock;
   mutable std::unique_ptr<BugType> BT_destroylock;
   mutable std::unique_ptr<BugType> BT_initlock;
   mutable std::unique_ptr<BugType> BT_lor;
   enum LockingSemantics {
     NotApplicable = 0,
     PthreadSemantics,
     XNUSemantics
   };
-public:
-  void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
-  void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
-  void printState(raw_ostream &Out, ProgramStateRef State,
-                  const char *NL, const char *Sep) const override;
 
-  void AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock,
-                   bool isTryLock, enum LockingSemantics semantics) const;
+  typedef void (PthreadLockChecker::*FnCheck)(const CallEvent &Call,
+                                              CheckerContext &C) const;
+  struct SupportedAPI {
+    CallDescription Name;
+    FnCheck Callback;
+  };
+  typedef SmallVector<SupportedAPI, 32> SupportedAPIList;
+  SupportedAPIList SupportedAPIs;
 
-  void ReleaseLock(CheckerContext &C, const CallExpr *CE, SVal lock) const;
-  void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock,
-                   enum LockingSemantics semantics) const;
-  void InitLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
-  void reportUseDestroyedBug(CheckerContext &C, const CallExpr *CE) const;
   ProgramStateRef resolvePossiblyDestroyedMutex(ProgramStateRef state,
                                                 const MemRegion *lockR,
                                                 const SymbolRef *sym) const;
+  void reportUseDestroyedBug(const CallEvent &Call, CheckerContext &C,
+                             unsigned ArgNo) const;
+
+  // Init.
+  void InitAnyLock(const CallEvent &Call, CheckerContext &C) const;
+  void InitLockAux(const CallEvent &Call, CheckerContext &C, unsigned ArgNo,
+                   SVal Lock) const;
+
+  // Lock, Try-lock.
+  void AcquirePthreadLock(const CallEvent &Call, CheckerContext &C) const;
+  void AcquireXNULock(const CallEvent &Call, CheckerContext &C) const;
+  void TryPthreadLock(const CallEvent &Call, CheckerContext &C) const;
+  void TryXNULock(const CallEvent &Call, CheckerContext &C) const;
+  void AcquireLockAux(const CallEvent &Call, CheckerContext &C, unsigned ArgNo,
+                      SVal lock, bool isTryLock,
+                      enum LockingSemantics semantics) const;
+
+  // Release.
+  void ReleaseAnyLock(const CallEvent &Call, CheckerContext &C) const;
+  void ReleaseLockAux(const CallEvent &Call, CheckerContext &C, unsigned ArgNo,
+                      SVal lock) const;
+
+  // Destroy.
+  void DestroyPthreadLock(const CallEvent &Call, CheckerContext &C) const;
+  void DestroyXNULock(const CallEvent &Call, CheckerContext &C) const;
+  void DestroyLockAux(const CallEvent &Call, CheckerContext &C, unsigned ArgNo,
+                      SVal Lock, enum LockingSemantics semantics) const;
+
+public:
+  PthreadLockChecker();
+
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+  void printState(raw_ostream &Out, ProgramStateRef State,
+                  const char *NL, const char *Sep) const override;
 };
 } // end anonymous namespace
 
 // A stack of locks for tracking lock-unlock order.
 REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
 
 // An entry for tracking lock states.
 REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)
 
 // Return values for unresolved calls to pthread_mutex_destroy().
 REGISTER_MAP_WITH_PROGRAMSTATE(DestroyRetVal, const MemRegion *, SymbolRef)
 
-void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
+PthreadLockChecker::PthreadLockChecker() : SupportedAPIs({
+  // Init.
+  {{"pthread_mutex_init",        2}, &PthreadLockChecker::InitAnyLock},
+  // TODO: pthread_rwlock_init(2 arguments).
+  // TODO: lck_mtx_init(3 arguments).
+  // TODO: lck_mtx_alloc_init(2 arguments) => returns the mutex.
+  // TODO: lck_rw_init(3 arguments).
+  // TODO: lck_rw_alloc_init(2 arguments) => returns the mutex.
+
+  // Acquire.
+  {{"pthread_mutex_lock",        1}, &PthreadLockChecker::AcquirePthreadLock},
+  {{"pthread_rwlock_rdlock",     1}, &PthreadLockChecker::AcquirePthreadLock},
+  {{"pthread_rwlock_wrlock",     1}, &PthreadLockChecker::AcquirePthreadLock},
+  {{"lck_mtx_lock",              1}, &PthreadLockChecker::AcquireXNULock},
+  {{"lck_rw_lock_exclusive",     1}, &PthreadLockChecker::AcquireXNULock},
+  {{"lck_rw_lock_shared",        1}, &PthreadLockChecker::AcquireXNULock},
+
+  // Try.
+  {{"pthread_mutex_trylock",     1}, &PthreadLockChecker::TryPthreadLock},
+  {{"pthread_rwlock_tryrdlock",  1}, &PthreadLockChecker::TryPthreadLock},
+  {{"pthread_rwlock_trywrlock",  1}, &PthreadLockChecker::TryPthreadLock},
+  {{"lck_mtx_try_lock",          1}, &PthreadLockChecker::TryXNULock},
+  {{"lck_rw_try_lock_exclusive", 1}, &PthreadLockChecker::TryXNULock},
+  {{"lck_rw_try_lock_shared",    1}, &PthreadLockChecker::TryXNULock},
+
+  // Release.
+  {{"pthread_mutex_unlock",      1}, &PthreadLockChecker::ReleaseAnyLock},
+  {{"pthread_rwlock_unlock",     1}, &PthreadLockChecker::ReleaseAnyLock},
+  {{"lck_mtx_unlock",            1}, &PthreadLockChecker::ReleaseAnyLock},
+  {{"lck_rw_unlock_exclusive",   1}, &PthreadLockChecker::ReleaseAnyLock},
+  {{"lck_rw_unlock_shared",      1}, &PthreadLockChecker::ReleaseAnyLock},
+  {{"lck_rw_done",               1}, &PthreadLockChecker::ReleaseAnyLock},
+
+  // Destroy.
+  {{"pthread_mutex_destroy",     1}, &PthreadLockChecker::DestroyPthreadLock},
+  {{"lck_mtx_destroy",           2}, &PthreadLockChecker::DestroyXNULock},
+  // TODO: pthread_rwlock_destroy(1 argument).
+  // TODO: lck_rw_destroy(2 arguments).
+}) {}
+
+void PthreadLockChecker::checkPostCall(const CallEvent &Call,
                                        CheckerContext &C) const {
-  ProgramStateRef state = C.getState();
-  const LocationContext *LCtx = C.getLocationContext();
-  StringRef FName = C.getCalleeName(CE);
-  if (FName.empty())
-    return;
-
-  if (CE->getNumArgs() != 1 && CE->getNumArgs() != 2)
-    return;
-
-  if (FName == "pthread_mutex_lock" ||
-      FName == "pthread_rwlock_rdlock" ||
-      FName == "pthread_rwlock_wrlock")
-    AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx),
-                false, PthreadSemantics);
-  else if (FName == "lck_mtx_lock" ||
-           FName == "lck_rw_lock_exclusive" ||
-           FName == "lck_rw_lock_shared")
-    AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx),
-                false, XNUSemantics);
-  else if (FName == "pthread_mutex_trylock" ||
-           FName == "pthread_rwlock_tryrdlock" ||
-           FName == "pthread_rwlock_trywrlock")
-    AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx),
-                true, PthreadSemantics);
-  else if (FName == "lck_mtx_try_lock" ||
-           FName == "lck_rw_try_lock_exclusive" ||
-           FName == "lck_rw_try_lock_shared")
-    AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx),
-                true, XNUSemantics);
-  else if (FName == "pthread_mutex_unlock" ||
-           FName == "pthread_rwlock_unlock" ||
-           FName == "lck_mtx_unlock" ||
-           FName == "lck_rw_unlock_exclusive" ||
-           FName == "lck_rw_unlock_shared" ||
-           FName == "lck_rw_done")
-    ReleaseLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
-  else if (FName == "pthread_mutex_destroy")
-    DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), PthreadSemantics);
-  else if (FName == "lck_mtx_destroy")
-    DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), XNUSemantics);
-  else if (FName == "pthread_mutex_init")
-    InitLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
+  if (!Call.isGlobalCFunction())
+    return;
+
+  for (const auto &I: SupportedAPIs) {
+    if (Call.isCalled(I.Name))
+      (this->*I.Callback)(Call, C);
+  }
 }
 
 // When a lock is destroyed, in some semantics(like PthreadSemantics) we are not
 // sure if the destroy call has succeeded or failed, and the lock enters one of
 // the 'possibly destroyed' state. There is a short time frame for the
 // programmer to check the return value to see if the lock was successfully
 // destroyed. Before we model the next operation over that lock, we call this
 // function to see if the return value was checked by now and set the lock state
for (auto I: LS) {
       I->dumpToStream(Out);
       Out << NL;
     }
   }
 
   // TODO: Dump destroyed mutex symbols?
 }
 
-void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE,
+void PthreadLockChecker::AcquirePthreadLock(const CallEvent &Call,
+                                            CheckerContext &C) const {
+  AcquireLockAux(Call, C, 0, Call.getArgSVal(0), false, PthreadSemantics);
+}
+
+void PthreadLockChecker::AcquireXNULock(const CallEvent &Call,
+                                           CheckerContext &C) const {
+  AcquireLockAux(Call, C, 0, Call.getArgSVal(0), false, XNUSemantics);
+}
+
+void PthreadLockChecker::TryPthreadLock(const CallEvent &Call,
+                                        CheckerContext &C) const {
+  AcquireLockAux(Call, C, 0, Call.getArgSVal(0), true, PthreadSemantics);
+}
+
+void PthreadLockChecker::TryXNULock(const CallEvent &Call,
+                                        CheckerContext &C) const {
+  AcquireLockAux(Call, C, 0, Call.getArgSVal(0), true, PthreadSemantics);
+}
+
+void PthreadLockChecker::AcquireLockAux(const CallEvent &Call,
+                                        CheckerContext &C, unsigned ArgNo,
                                         SVal lock, bool isTryLock,
                                         enum LockingSemantics semantics) const {
 
   const MemRegion *lockR = lock.getAsRegion();
   if (!lockR)
     return;
 
   ProgramStateRef state = C.getState();
   const SymbolRef *sym = state->get<DestroyRetVal>(lockR);
   if (sym)
     state = resolvePossiblyDestroyedMutex(state, lockR, sym);
 
   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 = llvm::make_unique<BugReport>(
           *BT_doublelock, "This lock has already been acquired", N);
-      report->addRange(CE->getArg(0)->getSourceRange());
+      report->addRange(Call.getArgExpr(ArgNo)->getSourceRange());
       C.emitReport(std::move(report));
       return;
     } else if (LState->isDestroyed()) {
-      reportUseDestroyedBug(C, CE);
+      reportUseDestroyedBug(Call, C, ArgNo);
       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());
+    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);
     }
     // 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.
-    SVal RetVal = state->getSVal(CE, C.getLocationContext());
+    SVal RetVal = Call.getReturnValue();
     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);
   lockSucc = lockSucc->set<LockMap>(lockR, LockState::getLocked());
   C.addTransition(lockSucc);
 }
 
-void PthreadLockChecker::ReleaseLock(CheckerContext &C, const CallExpr *CE,
+void PthreadLockChecker::ReleaseAnyLock(const CallEvent &Call,
+                                        CheckerContext &C) const {
+  ReleaseLockAux(Call, C, 0, Call.getArgSVal(0));
+}
+
+void PthreadLockChecker::ReleaseLockAux(const CallEvent &Call,
+                                        CheckerContext &C, unsigned ArgNo,
                                         SVal lock) const {
 
   const MemRegion *lockR = lock.getAsRegion();
   if (!lockR)
     return;
 
   ProgramStateRef state = C.getState();
   const SymbolRef *sym = state->get<DestroyRetVal>(lockR);
   if (sym)
     state = resolvePossiblyDestroyedMutex(state, lockR, sym);
 
   if (const LockState *LState = state->get<LockMap>(lockR)) {
     if (LState->isUnlocked()) {
       if (!BT_doubleunlock)
         BT_doubleunlock.reset(new BugType(this, "Double unlocking",
                                           "Lock checker"));
       ExplodedNode *N = C.generateErrorNode();
       if (!N)
         return;
       auto Report = llvm::make_unique<BugReport>(
           *BT_doubleunlock, "This lock has already been unlocked", N);
-      Report->addRange(CE->getArg(0)->getSourceRange());
+      Report->addRange(Call.getArgExpr(ArgNo)->getSourceRange());
       C.emitReport(std::move(Report));
       return;
     } else if (LState->isDestroyed()) {
-      reportUseDestroyedBug(C, CE);
+      reportUseDestroyedBug(Call, C, ArgNo);
       return;
     }
   }
 
   LockSetTy LS = state->get<LockSet>();
 
-  // FIXME: Better analysis requires IPA for wrappers.
-
   if (!LS.isEmpty()) {
     const MemRegion *firstLockR = LS.getHead();
     if (firstLockR != lockR) {
       if (!BT_lor)
         BT_lor.reset(new BugType(this, "Lock order reversal", "Lock checker"));
       ExplodedNode *N = C.generateErrorNode();
       if (!N)
         return;
       auto report = llvm::make_unique<BugReport>(
           *BT_lor, "This was not the most recently acquired lock. Possible "
                    "lock order reversal", N);
-      report->addRange(CE->getArg(0)->getSourceRange());
+      report->addRange(Call.getArgExpr(ArgNo)->getSourceRange());
       C.emitReport(std::move(report));
       return;
     }
     // Record that the lock was released.
     state = state->set<LockSet>(LS.getTail());
   }
 
   state = state->set<LockMap>(lockR, LockState::getUnlocked());
   C.addTransition(state);
 }
 
-void PthreadLockChecker::DestroyLock(CheckerContext &C, const CallExpr *CE,
+void PthreadLockChecker::DestroyPthreadLock(const CallEvent &Call,
+                                            CheckerContext &C) const {
+  DestroyLockAux(Call, C, 0, Call.getArgSVal(0), PthreadSemantics);
+}
+
+void PthreadLockChecker::DestroyXNULock(const CallEvent &Call,
+                                            CheckerContext &C) const {
+  DestroyLockAux(Call, C, 0, Call.getArgSVal(0), XNUSemantics);
+}
+
+void PthreadLockChecker::DestroyLockAux(const CallEvent &Call,
+                                        CheckerContext &C, unsigned ArgNo,
                                         SVal Lock,
                                         enum LockingSemantics semantics) const {
 
   const MemRegion *LockR = Lock.getAsRegion();
   if (!LockR)
     return;
 
   ProgramStateRef State = C.getState();
 
   const SymbolRef *sym = State->get<DestroyRetVal>(LockR);
   if (sym)
     State = resolvePossiblyDestroyedMutex(State, LockR, sym);
 
   const LockState *LState = State->get<LockMap>(LockR);
   // Checking the return value of the destroy method only in the case of
   // PthreadSemantics
   if (semantics == PthreadSemantics) {
     if (!LState || LState->isUnlocked()) {
-      SymbolRef sym = C.getSVal(CE).getAsSymbol();
+      SymbolRef sym = Call.getReturnValue().getAsSymbol();
       if (!sym) {
         State = State->remove<LockMap>(LockR);
         C.addTransition(State);
         return;
       }
       State = State->set<DestroyRetVal>(LockR, sym);
       if (LState && LState->isUnlocked())
         State = State->set<LockMap>(
void PthreadLockChecker::DestroyLockAux(const CallEvent &Call,
 
   if (!BT_destroylock)
     BT_destroylock.reset(new BugType(this, "Destroy invalid lock",
                                      "Lock checker"));
   ExplodedNode *N = C.generateErrorNode();
   if (!N)
     return;
   auto Report = llvm::make_unique<BugReport>(*BT_destroylock, Message, N);
-  Report->addRange(CE->getArg(0)->getSourceRange());
+  Report->addRange(Call.getArgExpr(ArgNo)->getSourceRange());
   C.emitReport(std::move(Report));
 }
 
-void PthreadLockChecker::InitLock(CheckerContext &C, const CallExpr *CE,
-                                  SVal Lock) const {
+void PthreadLockChecker::InitAnyLock(const CallEvent &Call,
+                                     CheckerContext &C) const {
+  InitLockAux(Call, C, 0, Call.getArgSVal(0));
+}
+
+void PthreadLockChecker::InitLockAux(const CallEvent &Call, CheckerContext &C,
+                                     unsigned ArgNo, SVal Lock) const {
 
   const MemRegion *LockR = Lock.getAsRegion();
   if (!LockR)
     return;
 
   ProgramStateRef State = C.getState();
 
   const SymbolRef *sym = State->get<DestroyRetVal>(LockR);
void PthreadLockChecker::InitLockAux(const CallEvent &Call, CheckerContext &C,
 
   if (!BT_initlock)
     BT_initlock.reset(new BugType(this, "Init invalid lock",
                                   "Lock checker"));
   ExplodedNode *N = C.generateErrorNode();
   if (!N)
     return;
   auto Report = llvm::make_unique<BugReport>(*BT_initlock, Message, N);
-  Report->addRange(CE->getArg(0)->getSourceRange());
+  Report->addRange(Call.getArgExpr(ArgNo)->getSourceRange());
   C.emitReport(std::move(Report));
 }
 
-void PthreadLockChecker::reportUseDestroyedBug(CheckerContext &C,
-                                               const CallExpr *CE) const {
+void PthreadLockChecker::reportUseDestroyedBug(const CallEvent &Call,
+                                               CheckerContext &C,
+                                               unsigned ArgNo) const {
   if (!BT_destroylock)
     BT_destroylock.reset(new BugType(this, "Use destroyed lock",
                                      "Lock checker"));
   ExplodedNode *N = C.generateErrorNode();
   if (!N)
     return;
   auto Report = llvm::make_unique<BugReport>(
       *BT_destroylock, "This lock has already been destroyed", N);
-  Report->addRange(CE->getArg(0)->getSourceRange());
+  Report->addRange(Call.getArgExpr(ArgNo)->getSourceRange());
   C.emitReport(std::move(Report));
 }
 
 void PthreadLockChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                           CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
   // TODO: Clean LockMap when a mutex region dies.