pthread_mutex_lock() modeling error

[Quuxplusone]

Bugzilla Link	PR8395
Status	NEW
Importance	P normal
Reported by	Dmitry G. Dyachenko (dimhen@gmail.com)
Reported on	2010-10-17 05:21:34 -0700
Last modified on	2012-10-22 12:27:35 -0700
Version	trunk
Hardware	PC Linux
CC	dimhen@gmail.com, jrose@belkadan.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

$ clang -v
clang version 2.9 (trunk 116687)
Target: x86_64-unknown-linux-gnu
Thread model: posix
s$ cat pthread_mutex_lock.c
#include <pthread.h>

static pthread_mutex_t m;

void foo()
{
    int err = 0;
    err = pthread_mutex_lock(&m);
    if(err)
        return;
    pthread_mutex_unlock(&m);
}
$ clang -cc1 pthread_mutex_lock.c -analyze -analyzer-check-objc-mem -analyzer-
experimental-checks
pthread_mutex_lock.c:10:2: warning: This statement is never executed
        return;
        ^~~~~~
1 warning generated.

[Quuxplusone]

Both functions -- pthread_mutex_lock() and pthread_mutex_trylock() can:
- acquire lock
- return error

Then we can process them equally.
And the following naive patch will solve PR

- remove now useless 'bool isTryLock' from AcquireLock()
- remove now useless else-part from 'if(isTryLock)'

- Patch was tested with PR testcase only
- Patch is against rev.120936

$ clang --version
clang version 2.9 (trunk 120936)
Target: x86_64-unknown-linux-gnu
Thread model: posix

Checker$ svn diff
Index: PthreadLockChecker.cpp
===================================================================
--- PthreadLockChecker.cpp      (revision 120936)
+++ PthreadLockChecker.cpp      (working copy)
@@ -32,8 +32,7 @@
   }
   void PostVisitCallExpr(CheckerContext &C, const CallExpr *CE);

-  void AcquireLock(CheckerContext &C, const CallExpr *CE,
-                   SVal lock, bool isTryLock);
+  void AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock);

   void ReleaseLock(CheckerContext &C, const CallExpr *CE,
                     SVal lock);
@@ -73,12 +72,12 @@
   if (FName == "pthread_mutex_lock") {
     if (CE->getNumArgs() != 1)
       return;
-    AcquireLock(C, CE, state->getSVal(CE->getArg(0)), false);
+    AcquireLock(C, CE, state->getSVal(CE->getArg(0)));
   }
   else if (FName == "pthread_mutex_trylock") {
     if (CE->getNumArgs() != 1)
       return;
-    AcquireLock(C, CE, state->getSVal(CE->getArg(0)), true);
+    AcquireLock(C, CE, state->getSVal(CE->getArg(0)));
   }
   else if (FName == "pthread_mutex_unlock") {
     if (CE->getNumArgs() != 1)
@@ -88,7 +87,7 @@
 }

 void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE,
-                                     SVal lock, bool isTryLock) {
+                                     SVal lock) {

   const MemRegion *lockR = lock.getAsRegion();
   if (!lockR)
@@ -103,18 +102,11 @@
   DefinedSVal retVal = cast<DefinedSVal>(X);
   const GRState *lockSucc = state;

-  if (isTryLock) {
-      // Bifurcate the state, and allow a mode where the lock acquisition
fails.
-    const GRState *lockFail;
-    llvm::tie(lockFail, lockSucc) = state->assume(retVal);
-    assert(lockFail && lockSucc);
-    C.addTransition(C.GenerateNode(CE, lockFail));
-  }
-  else {
-      // Assume that the return value was 0.
-    lockSucc = state->assume(retVal, false);
-    assert(lockSucc);
-  }
+    // Bifurcate the state, and allow a mode where the lock acquisition fails.
+  const GRState *lockFail;
+  llvm::tie(lockFail, lockSucc) = state->assume(retVal);
+  assert(lockFail && lockSucc);
+  C.addTransition(C.GenerateNode(CE, lockFail));

     // Record that the lock was acquired.
   lockSucc = lockSucc->add<LockSet>(lockR);

[Quuxplusone]

$ clang --version
clang version 3.2 (trunk 165709)
Target: x86_64-unknown-linux-gnu
Thread model: posix

PASS for me. Thanks!

[Quuxplusone]

This passed because we turned off the analyzer's dead code checker for now. It still fails with

% clang -cc1 -analyze -analyzer-checker=alpha.unix.PthreadLock,alpha.deadcode pthread_mutex_lock.c

Arguably there's a policy decision here: many people do not expect pthread locks to fail. We probably need to do something similar to realloc -- most people just assume it succeeds, but if someone was defensive enough to test it then we can offer leak warnings on the original buffer.