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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
