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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

It seems @phosek was able to fix the issue in 
https://github.com/flutter/engine/pull/5944. By the way, a nice way to think 
about the attributes is that they encode state transitions as shown in the 
following table. This change fills the remaining two gaps.

|  | unlocked | locked exclusive | locked  shared|
| unlocked/unknown | `EXCLUDES`   | `ACQUIRE`| `ACQUIRE_SHARED`  |
| locked exclusive | `RELEASE`| `REQUIRES`   |   |
| locked shared| `RELEASE_SHARED` |  | `REQUIRES_SHARED` |
|

If more people stumble into the issue, another approach would be possible. My 
understanding is that the order of attributes is preserved. So we could treat 
`ACQUIRE(m) RELEASE(m)` = `EXCLUDES(m)` differently than `RELEASE(m) 
ACQUIRE(m)` = `REQUIRES(m)`. But I'm not sure if that's desirable, since 
normally the order of attributes does not matter.


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

[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` 
> 
>  calls `LookupIsolatePortByNameUnprotected` 
> 
>  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

[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` 

 calls `LookupIsolatePortByNameUnprotected` 

 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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.


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

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

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

This change broke the acquire+release case. Concretely, in Flutter we have this 
code: 
https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server.h#L26.
 This worked fine previously, but after this change we're getting an error in 
https://github.com/flutter/engine/blob/master/lib/ui/isolate_name_server/isolate_name_server_natives.cc#L19
 and many other places like this one:

  
../../third_party/flutter/lib/ui/isolate_name_server/isolate_name_server_natives.cc:19:33:
 error: releasing mutex 'name_server->mutex_' that was not held 
[-Werror,-Wthread-safety-analysis]
Dart_Port port = name_server->LookupIsolatePortByName(name);
  ^
  
../../third_party/flutter/lib/ui/isolate_name_server/isolate_name_server_natives.cc:24:1:
 error: mutex 'name_server->mutex_' is still held at the end of function 
[-Werror,-Wthread-safety-analysis]
  }
  ^
  
../../third_party/flutter/lib/ui/isolate_name_server/isolate_name_server_natives.cc:19:33:
 note: mutex acquired here
Dart_Port port = name_server->LookupIsolatePortByName(name);

Would it be possible revert this change? The old logic was "all acquires; then 
all releases" and the new logic is "all releases; then all acquires" but I 
think this needs fancier logic with actual bookkeeping to detect the upgrade 
case without breaking the acquire+release case.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

I've commit in r338024, thank you for the patch!


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

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

No problem. Thanks for the review! Would be nice if you or @aaron.ballman could 
commit this, as I don't have commit access.


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

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

[Delesley Hutchins via Phabricator via cfe-commits]

delesley accepted this revision.
delesley added a comment.
This revision is now accepted and ready to land.

Looks good to me.  Thanks for the patch, and my apologies for the slow 
response.  (I'm on a different project now, so I'm afraid thread safety doesn't 
always get the attention from me that it deserves.)

  -DeLesley


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'm going to let @delesley give the final sign off on this as he is more 
familiar with this part of the analysis, but I think this looks reasonable.


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

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Ping? The functional changes should be minimal.




Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:38-39
 #endif
+#define EXCLUSIVE_UNLOCK_FUNCTION(...)  
__attribute__((release_capability(__VA_ARGS__)))
+#define SHARED_UNLOCK_FUNCTION(...) 
__attribute__((release_shared_capability(__VA_ARGS__)))
 #define UNLOCK_FUNCTION(...)
__attribute__((unlock_function(__VA_ARGS__)))

This was necessary to make sure we produce warnings of the kind `releasing 
mutex '...' using exclusive access, expected shared access` for both 
configurations.


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

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
Herald added a subscriber: cfe-commits.

We can now have methods that release a locked in shared mode and acquire
it in exclusive mode or the other way around. The fix was just to
release the locks before acquiring them.

Also added a few test cases for non-generic unlock methods and removed
an unnecessary const_cast.


Repository:
  rC Clang

https://reviews.llvm.org/D49355

Files:
  lib/Analysis/ThreadSafety.cpp
  test/SemaCXX/warn-thread-safety-analysis.cpp

Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -22,8 +22,6 @@
 #define SHARED_LOCK_FUNCTION(...)   __attribute__((acquire_shared_capability(__VA_ARGS__)))
 #define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((try_acquire_capability(__VA_ARGS__)))
 #define SHARED_TRYLOCK_FUNCTION(...)__attribute__((try_acquire_shared_capability(__VA_ARGS__)))
-#define EXCLUSIVE_UNLOCK_FUNCTION(...)  __attribute__((release_capability(__VA_ARGS__)))
-#define SHARED_UNLOCK_FUNCTION(...) __attribute__((release_shared_capability(__VA_ARGS__)))
 #define EXCLUSIVE_LOCKS_REQUIRED(...)   __attribute__((requires_capability(__VA_ARGS__)))
 #define SHARED_LOCKS_REQUIRED(...)  __attribute__((requires_shared_capability(__VA_ARGS__)))
 #else
@@ -34,11 +32,11 @@
 #define SHARED_LOCK_FUNCTION(...)   __attribute__((shared_lock_function(__VA_ARGS__)))
 #define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((exclusive_trylock_function(__VA_ARGS__)))
 #define SHARED_TRYLOCK_FUNCTION(...)__attribute__((shared_trylock_function(__VA_ARGS__)))
-#define EXCLUSIVE_UNLOCK_FUNCTION(...)  __attribute__((unlock_function(__VA_ARGS__)))
-#define SHARED_UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__)))
 #define EXCLUSIVE_LOCKS_REQUIRED(...)   __attribute__((exclusive_locks_required(__VA_ARGS__)))
 #define SHARED_LOCKS_REQUIRED(...)  __attribute__((shared_locks_required(__VA_ARGS__)))
 #endif
+#define EXCLUSIVE_UNLOCK_FUNCTION(...)  __attribute__((release_capability(__VA_ARGS__)))
+#define SHARED_UNLOCK_FUNCTION(...) __attribute__((release_shared_capability(__VA_ARGS__)))
 #define UNLOCK_FUNCTION(...)__attribute__((unlock_function(__VA_ARGS__)))
 #define LOCK_RETURNED(x)__attribute__((lock_returned(x)))
 #define LOCKS_EXCLUDED(...) __attribute__((locks_excluded(__VA_ARGS__)))
@@ -50,10 +48,15 @@
   void Lock() EXCLUSIVE_LOCK_FUNCTION();
   void ReaderLock() SHARED_LOCK_FUNCTION();
   void Unlock() UNLOCK_FUNCTION();
+  void ExclusiveUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+  void ReaderUnlock() SHARED_UNLOCK_FUNCTION();
   bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true);
   bool ReaderTryLock() SHARED_TRYLOCK_FUNCTION(true);
   void LockWhen(const int ) EXCLUSIVE_LOCK_FUNCTION();
 
+  void PromoteShared() SHARED_UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() EXCLUSIVE_UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+
   // for negative capabilities
   const Mutex& operator!() const { return *this; }
 
@@ -704,6 +707,26 @@
   sls_mu.Unlock();
 }
 
+void shared_fun_9() {
+  sls_mu.Lock();
+  sls_mu.ExclusiveUnlock();
+
+  sls_mu.ReaderLock();
+  sls_mu.ReaderUnlock();
+}
+
+void shared_fun_10() {
+  sls_mu.Lock();
+  sls_mu.DemoteExclusive();
+  sls_mu.ReaderUnlock();
+}
+
+void shared_fun_11() {
+  sls_mu.ReaderLock();
+  sls_mu.PromoteShared();
+  sls_mu.Unlock();
+}
+
 void shared_bad_0() {
   sls_mu.Lock();  // \
 // expected-warning {{mutex 'sls_mu' is acquired exclusively and shared in the same scope}}
@@ -737,6 +760,32 @@
   sls_mu.Unlock();
 }
 
+void shared_bad_3() {
+  sls_mu.Lock();
+  sls_mu.ReaderUnlock(); // \
+// expected-warning {{releasing mutex 'sls_mu' using shared access, expected exclusive access}}
+}
+
+void shared_bad_4() {
+  sls_mu.ReaderLock();
+  sls_mu.ExclusiveUnlock(); // \
+// expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
+}
+
+void shared_bad_5() {
+  sls_mu.Lock();
+  sls_mu.PromoteShared(); // \
+// expected-warning {{releasing mutex 'sls_mu' using shared access, expected exclusive access}}
+  sls_mu.ExclusiveUnlock();
+}
+
+void shared_bad_6() {
+  sls_mu.ReaderLock();
+  sls_mu.DemoteExclusive(); // \
+// expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
+  sls_mu.ReaderUnlock();
+}
+
 // FIXME: Add support for functions (not only methods)
 class LRBar {
  public:
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -109,9 +109,7 @@
 /// along with additional information, such as where it was acquired, whether
 /// it is exclusive or shared, etc.
 ///
-/// FIXME: this