[analyzer] Move function declarations of PthreadLockChecker's tests to a separate system-like header

[a.sidorin]

Summary

Regions passed to pthread* functions are invalidated in tests because the call is evaluated conservatively and declarations are not located in system header. This patch creates a system-like header with declarations previously located in test file.

[krememek]

This feels like a big hammer to wield just to prevent the conservative invalidation done by the static analyzer for calls to functions whose bodies are not available.

I feel like a more general approach might be to provide new hooks for checkers to provide hints to the core analyzer engine. For example, a callback that indicates whether or not invalidation should take place. That seems like a solution that could be utilized by other checkers besides just this one, and would allow checkers to opt into this with very little code.

[jordan_rose]

I tend to agree. eval::Call has the downside that only one checker can provide an implementation (since it controls the return value). But I'm also surprised that the example is failing—because the pthread_* methods are declared in system headers, only globals declared in system headers should be invalidated.

[a.sidorin]

You're right Jordan. I have made wrong suggestions from my investigation. It should be tests that should be fixed, not the checker itself.

[a.sidorin]

Fix tests, not the checker itself. Sorry for mistake and thanks to Jordan for help.

[jordan_rose]

Looks good, committed in r217515.

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

   }
 };
 
-class PthreadLockChecker : public Checker< check::PostStmt<CallExpr> > {
+class PthreadLockChecker : public Checker< eval::Call,
+                                           check::PostStmt<CallExpr> > {
   mutable std::unique_ptr<BugType> BT_doublelock;
   mutable std::unique_ptr<BugType> BT_doubleunlock;
   mutable std::unique_ptr<BugType> BT_destroylock;

   };
 public:
   void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
-    
+  bool evalCall(const CallExpr *CE, CheckerContext &C) const;
+
   void AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock,
                    bool isTryLock, enum LockingSemantics semantics) const;
     

 
 REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)
 
-void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
-                                       CheckerContext &C) const {
+bool PthreadLockChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
   ProgramStateRef state = C.getState();
   const LocationContext *LCtx = C.getLocationContext();
   StringRef FName = C.getCalleeName(CE);
   if (FName.empty())
-    return;
+    return false;
 
   if (CE->getNumArgs() != 1 && CE->getNumArgs() != 2)
-    return;
+    return false;
 
   if (FName == "pthread_mutex_lock" ||
       FName == "pthread_rwlock_rdlock" ||
-      FName == "pthread_rwlock_wrlock")
+      FName == "pthread_rwlock_wrlock") {
     AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx),
                 false, PthreadSemantics);
-  else if (FName == "lck_mtx_lock" ||
+    return true;
+  } else if (FName == "lck_mtx_lock" ||
            FName == "lck_rw_lock_exclusive" ||
-           FName == "lck_rw_lock_shared") 
+           FName == "lck_rw_lock_shared") {
     AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx),
                 false, XNUSemantics);
-  else if (FName == "pthread_mutex_trylock" ||
+    return true;
+  } else if (FName == "pthread_mutex_trylock" ||
            FName == "pthread_rwlock_tryrdlock" ||
-           FName == "pthread_rwlock_trywrlock")
+           FName == "pthread_rwlock_trywrlock") {
     AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx),
                 true, PthreadSemantics);
-  else if (FName == "lck_mtx_try_lock" ||
+    return true;
+  } else if (FName == "lck_mtx_try_lock" ||
            FName == "lck_rw_try_lock_exclusive" ||
-           FName == "lck_rw_try_lock_shared")
+           FName == "lck_rw_try_lock_shared") {
     AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx),
                 true, XNUSemantics);
-  else if (FName == "pthread_mutex_unlock" ||
+    return true;
+  } else if (FName == "pthread_mutex_unlock" ||
            FName == "pthread_rwlock_unlock" ||
            FName == "lck_mtx_unlock" ||
-           FName == "lck_rw_done")
+           FName == "lck_rw_done") {
     ReleaseLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
-  else if (FName == "pthread_mutex_destroy" ||
-           FName == "lck_mtx_destroy")
+    return true;
+  }
+  return false;
+}
+
+
+void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
+                                       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_destroy" ||
+      FName == "lck_mtx_destroy")
     DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
   else if (FName == "pthread_mutex_init")
     InitLock(C, CE, state->getSVal(CE->getArg(0), LCtx));

     return;
   
   ProgramStateRef state = C.getState();
-  
-  SVal X = state->getSVal(CE, C.getLocationContext());
-  if (X.isUnknownOrUndef())
-    return;
-  
-  DefinedSVal retVal = X.castAs<DefinedSVal>();
+  const LocationContext *LCtx = C.getLocationContext();
+  DefinedSVal retVal = C.getSValBuilder() .conjureSymbolVal(nullptr, CE, LCtx,
+                                                            C.blockCount())
+      .castAs<DefinedSVal>();
+  state = state->BindExpr(CE, LCtx, retVal);
 
   if (const LockState *LState = state->get<LockMap>(lockR)) {
     if (LState->isLocked()) {

   }
 
   state = state->set<LockMap>(lockR, LockState::getUnlocked());
+  const LocationContext *LCtx = C.getLocationContext();
+  DefinedSVal retVal = C.getSValBuilder().conjureSymbolVal(nullptr, CE, LCtx,
+                                                           C.blockCount())
+      .castAs<DefinedSVal>();
+  state = state->BindExpr(CE, LCtx, retVal);
   C.addTransition(state);
 }
 

test/Analysis/pthreadlock.c

 lck_mtx_t lck1, lck2;
 lck_grp_t grp1;
 
+pthread_mutex_t *pmtx;
+
 #define NULL 0
 
 void

 }
 
 void
+ok21(void) {
+  pthread_mutex_lock(pmtx); // no-warning
+  pthread_mutex_unlock(pmtx); // no-warning
+}
+
+void
+ok22(void) {
+  pthread_mutex_lock(pmtx); // no-warning
+  pthread_mutex_unlock(pmtx); // no-warning
+  pthread_mutex_lock(pmtx); // no-warning
+  pthread_mutex_unlock(pmtx); // no-warning
+}
+
+
+void
 bad1(void)
 {
   pthread_mutex_lock(&mtx1);  // no-warning