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

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D49355