[cfe-commits] PthreadLock Checker enhancements

[Jordy Rose]

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.


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"?

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.


Other than that, it looks good to me and ready to go in, unless Ted or anyone else has strong feelings about the PTHREAD_MUTEX_RECURSIVE issue:

>>  - Technically, POSIX mutexes can be recursive locks (see PTHREAD_MUTEX_RECURSIVE), which would produce false-positives in the double-locking case. I'm not sure what we'd want to do about this. :-) (At some point, I think we need a way to set arbitrary flags for checkers, or to see under what name a checker is enabled.)
> 
> Yes, this is not an easy problem to solve because the checker only runs on one file at a time and doesn't save any state between files. I've largely ignored this problem for now, but I will need to revisit it in the future.

Thanks for working on this!
Jordy