[PATCH] Teach pthread checker about double unlocks

[Jordan Rose]

On Mar 19, 2014, at 13:17 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

> Hi Jordan,
> 
> On Wed, 2014-03-19 at 10:20 -0700, Jordan Rose wrote:
>> Hi, Daniel. I'm wondering why we need a Locked state at all. We still
>> want to support an unbalanced unlock, and if we want to handle
>> recursive locks (PR10418) we'd have to do more work anyway than just
>> "locked" or "unlocked". (The existence of recursive locks means that a
>> double-unlock warning isn't always correct.)
>> 
>> 
>> I didn't look at the patch too closely, but it mostly looks sensible.
>> What do you think about this, though?
>> Jordan
> 
> You are right. The locked state isn't needed since that information
> already is in the list of current locks. But we must know if the lock is
> unlocked or not for the double detection to work, removing the entry
> from the map when locking it would do.

Right, at which point it's really just a set.

> 
> I'll go back to the drawing board and try to think of a way to address
> the recursive locks while at it. What is your opinion about default
> behavior regarding recursive locks?
> 
> int main(int argc, char *argv[])
> {
>  pthread_mutex_t mtx = PTHREAD_MUTEX_INITIALIZER;
>  pthread_mutex_lock(&mtx);
>  pthread_mutex_lock(&mtx);
>  return 0;
> }
> 
> will behave differently depending on the OS. On FreeBSD and Solaris the
> process will terminate while Linux and AIX will deadlock. How OS X
> behaves I do not know. Should the default be to allow recursive locking,
> not allow it, or depend on the current OS?
> 
> Btw, what do you mean with "to support an unbalanced unlock"?

void dispose(struct resource_t *rsrc) {
  free(rsrc->buffer);
  pthread_mutex_unlock(rsrc->buffer);
}

This kind of thing. In this case, we're trying to unlock something even though we've never seen it get locked. We shouldn't warn on this, just like we shouldn't warn on the free() here.


I'm not sure what to do about recursive locks. I can think of a few choices for how to control it:

1. Use an -analyzer-config option. Downside: a bit annoying to access from checkers, not necessarily something we want users to think about (at least not right now).
2. Have two variants of the checker, and allow the user to override the default by asking for the other one.
3. Don't allow the user to override our default.

...and then choices for our default:

1. Allow recursive locks (severely reduces checking).
2. Disallow recursive locks (false positives sometimes).
3. Disallow recursive locks on platforms where that's not the default.

(FWIW it looks like OS X also uses non-recursive locks by default. I didn't test it but I assume iOS does as well.)

I'm inclined to say that for now we should just continue pretending all locks are non-recursive, but possibly separate out the lock/unlock checking from the lock order checking, so that if your application makes heavy use of recursive locks you could just disable that part of the checker. What do you think?

Jordan

P.S. If we can see where a mutex is created, we could also record if it was created with the "recursive" flag. But I think most uses of locks aren't right next to the creation point.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140319/c15a2494/attachment.html>