[cfe-commits] PthreadLock Checker enhancements

[Rui Paulo]

Hi,

On Jul 14, 2011, at 4:56 PM, Jordy Rose wrote:

> Sorry I missed your last e-mails...my filters decided to start throwing away responses to e-mails on the Clang lists. I think I have it fixed now.
> 
> The changes you've made look good. A couple final comments: 
> 
>> +  for (llvm::ImmutableList<const MemRegion*>::iterator I = LS.begin(),
>> +    E = LS.end(); I != E; ++I) {
>> +    if (*I == lockR) {
>> +      isLocked = true;
>> +      break;
>> +    }
>> +  }
> 
> Your ImmutableList::contains is in now, so you can take the loop out. :-)
> 
> 
>> +    default:
>> +      llvm_unreachable(0);
>> +      break;
> 
> It'd be nice to put a message here, like "unknown tryLock locking semantics", so that if someone ever adds a new locking semantics type but forgets to update this they'll see the error message in the crash log.
> 
> 
>>   else {
>> -      // Assume that the return value was 0.
>> +    // Assume that the return value was 0.
>>     lockSucc = state->assume(retVal, false);
>>     assert(lockSucc);
>>   }
> 
> It'd be nice to note that although XNU and pthread semantics are different here, the return type of XNU's lock is void. Alternately, you could just do a little more work in checkPostStmt to get rid of the NotApplicable locking type, and have the lockingSemantics determine how to bind the return value.

Like this?

+  if (FName == "pthread_mutex_lock" ||
+      FName == "pthread_rwlock_rdlock" ||
+      FName == "pthread_rwlock_wrlock")
+    AcquireLock(C, CE, state->getSVal(CE->getArg(0)), 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)), false, XNUSemantics);

....

-  else {
-      // Assume that the return value was 0.
+  /* XNU locking semantics return void on non-try locks */
+  else if (lockingSemantics == PthreadSemantics) {
+    // Assume that the return value was 0.

> 
> 
> Finally, a purely aesthetic note: "This lock has already been locked" and "This was not the most recently locked lock" sound a little funny to me because of the use of 'lock' as both a noun and a verb. How about "This lock has already been acquired" and "This was not the most recently acquired lock"?

Fixed.

> 
> And for the part about a note where the lock was last acquired, I'm still learning about it myself, but essentially you'd be subclassing BugReporterVisitor to create a PathDiagnosticEventPiece when the lock was most recently added to the lockset (i.e., when the current node's lockset contains the lock and the previous node's lockset does not). But that can come later.

Yes, I don't have plans to add that on this patch.

Please see the attached patch.

Regards,
--
Rui Paulo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pthreadlock.diff
Type: application/octet-stream
Size: 11042 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110716/63ad51c2/attachment.obj>