[cfe-dev] clang static analyzer for PthreadLockChecker.cpp

[Jordan Rose]

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


On Nov 23, 2013, at 4:37 , Jean Lee <xiaoyur347 at gmail.com> wrote:

> 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.
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131204/b5766446/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PthreadLockChecker.cpp
Type: text/x-c++src
Size: 7830 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131204/b5766446/attachment.cpp>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131204/b5766446/attachment-0001.html>