clang static analyzer for PthreadLockChecker.cpp

Platform: ubuntu 12.04 i386 + clang 3.4 rc1(built by myself). Also in clang 3.3
Checker:-enable-checker alpha.unix.PthreadLock
The checker located in
llvm/tools/clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

I find the checker has false positive for "This was not the most
recently acquired lock. Possible lock order reversal".
Code is as follows:
pthread_mutex_lock(&(pNode->pIPCSocket->mutexHandle));
PushNodeMsg(pNode, pEvent); //lock + unlock other mutex
// send notify
pthread_cond_signal(&(pNode->pIPCSocket->condHandle));
pthread_mutex_unlock(&(pNode->pIPCSocket->mutexHandle));

I debug it myself with my modification listed in attached file and find
/opt/evpp/cbb/evideo/commu/ipc/trunk/src/ipc.c:502:9: warning: This
was not the most recently acquired lock. Possible lock order reversal
208741436 SymRegion{derived_$133{conj_$132{int},SymRegion{reg_$115<SymRegion{reg_$67<SymRegion{reg_$12<g_pNodeHead>}->pNext>}->pNext>}->pIPCSocket}}->mutexHandle
208753604 SymRegion{derived_$136{conj_$135{int},SymRegion{reg_$115<SymRegion{reg_$67<SymRegion{reg_$12<g_pNodeHead>}->pNext>}->pNext>}->pIPCSocket}}->mutexHandle
[0]SymRegion{derived_$133{conj_$132{int},SymRegion{reg_$115<SymRegion{reg_$67<SymRegion{reg_$12<g_pNodeHead>}->pNext>}->pNext>}->pIPCSocket}}->mutexHandle

I think maybe it is ugly to use
pthread_mutex_lock(&(pNode->pIPCSocket->mutexHandle));
...
pthread_mutex_unlock(&(pNode->pIPCSocket->mutexHandle));
and there is no false positive if I modify to
pthread_mutex_lock(&(pIPCSocket->mutexHandle));
...
pthread_mutex_unlock(&(pIPCSocket->mutexHandle));

So, I guess the code(PthreadLockChecker.cpp)
const MemRegion *firstLockR = LS.getHead();
  if (firstLockR != lockR) {
    //This was not the most recently acquired lock. Possible lock order reversal
  }
is insuffient.

PthreadLockChecker.cpp (7.65 KB)

Hm, that does seem like a problem:

pthread_mutex_lock(&(pNode->pIPCSocket->mutexHandle));
PushNodeMsg(pNode, pEvent); //lock + unlock other mutex
// send notify
pthread_cond_signal(&(pNode->pIPCSocket->condHandle));
pthread_mutex_unlock(&(pNode->pIPCSocket->mutexHandle));

The analyzer is being careful in saying that maybe PushNodeMsg could modify the mutexHandle via pNode, but unfortunately the PthreadLockChecker doesn’t react to that. Please file a bug at http://llvm.org/bugs/ with a self-contained test case (or at least a preprocessed file); alternately, patches welcome. The general idea would be to add a checkDeadSymbols callback and replace any now-invalidated lock symbols with placeholders, or something similar.

Problems like this this are why it’s still marked as an alpha checker. Hopefully someone will have time to clean it up at some point.

Thanks for tracking this down!
Jordan

PthreadLockChecker.cpp (7.65 KB)