[PATCH] D49355: Thread safety analysis: Allow lock upgrading and downgrading

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In https://reviews.llvm.org/D49355#1188603, @phosek wrote:

> In https://reviews.llvm.org/D49355#1188520, @aaronpuchert wrote:
>
> > Could you explain what annotations like `LOCK_UNLOCK` are useful for? What do they check? The annotation should certainly not be necessary.
> >
> > Shouldn't you just use `REQUIRES(!...)` or `EXCLUDES(...)`? If a function locks and unlocks a mutex, I don't see a reason to have annotations on it, other than for preventing double locks. But for that we have negative capabilities.
>
>
> The purpose here indeed is to avoid double locks. I tried using `EXCLUDES(...)` but that doesn't work because `RegisterIsolatePortWithName` <https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server.h#L31> calls `LookupIsolatePortByNameUnprotected` <https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server.h#L39> which has `EXCLUSIVE_LOCKS_REQUIRED(...)` annotation. I also tried using the negative annotation but that reports far too many warnings in the existing code which makes it unusable.
>
> I'm fine changing the code, but unless there's a simple workaround I'd still argue for a revert, because the change even if correct has broken an existing usage pattern that worked fine for a long time before and is used in large codebases.


Can you explain in more detail what doesn't work? If you lock `mutex_` in the former function, the analysis should register that anyway and don't complain about calling the latter. The `LOCK_UNLOCK` annotation should be equivalent to `EXCLUDES(mutex_)` in that regard, too. There should also not be a difference regarding callers of the former function, in both cases the attributes don't propagate.

You should be aware that `LOCK_UNLOCK` does **not** prevent double-locking. If that is your concern, use negative annotations, i.e. `REQUIRES(!mutex_)` and `-Wthread-safety-negative`. Yes, that likely produces some more warnings, but not outside the class since `mutex_` is a private member.

Having `LOCK_UNLOCK` annotations doesn't seem to be intended, because one should neutralize the other. There was no example in the documentation, and apparently also no test in `test/SemaCXX/warn-thread-safety-analysis.cpp`. You should use `EXCLUDES` instead, or if you care enough about avoiding double locking, `REQUIRES(!...)`.


Repository:
  rC Clang

https://reviews.llvm.org/D49355