Handle shared release attributes correctly

[aaron.ballman]

test/SemaCXX/warn-thread-safety-analysis.cpp

4081–4085

Nothing calls either of these functions within the test; is that intended?

[aaronpuchert]

test/SemaCXX/warn-thread-safety-analysis.cpp

4081–4085

Yes, I just wanted to check that there are no warnings within the functions. Before the changes in lib/Analysis/ThreadSafety.cpp, we would get a warning "releasing mutex 'mu' using shared access, expected exclusive access" on line 4086.

My changes address the attributes on the function being analyzed, not on a function that is called from the function being analyzed.

[aaron.ballman]

LGTM!

test/SemaCXX/warn-thread-safety-analysis.cpp

4081–4085

Ah, okay, thank you for the explanation. I wasn't aware that was previously failing, so I didn't see the behavioral change.

[aaronpuchert]

Thanks for the review! Could you commit for me again?

[aaron.ballman]

In D50110#1187771, @aaronpuchert wrote:

Thanks for the review! Could you commit for me again?

I can, but it might also be a good idea for you to obtain your own commit privileges at this point (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access), if that's something you're interested in. Let me know which route you'd like to go.

[aaronpuchert]

For now I think I'm done fixing things in the thread safety analysis, but if I see myself contributing more in the longer term, I will definitely try to obtain commit access.

[aaron.ballman]

In D50110#1187828, @aaronpuchert wrote:

For now I think I'm done fixing things in the thread safety analysis, but if I see myself contributing more in the longer term, I will definitely try to obtain commit access.

Sounds good to me. I've commit in r338912. Thank you for the thread safety analysis patches!

lib/Analysis/ThreadSafety.cpp

for (const auto *Attr : D->attrs()) {
         getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A,
                     nullptr, D);
         CapDiagKind = ClassifyDiagnostic(A);
       } else if (const auto *A = dyn_cast<ReleaseCapabilityAttr>(Attr)) {
         // UNLOCK_FUNCTION() is used to hide the underlying lock implementation.
         // We must ignore such methods.
         if (A->args_size() == 0)
           return;
-        // FIXME -- deal with exclusive vs. shared unlock functions?
-        getMutexIDs(ExclusiveLocksToAdd, A, nullptr, D);
+        getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A,
+                    nullptr, D);
         getMutexIDs(LocksReleased, A, nullptr, D);
         CapDiagKind = ClassifyDiagnostic(A);
       } else if (const auto *A = dyn_cast<AcquireCapabilityAttr>(Attr)) {
         if (A->args_size() == 0)
           return;
         getMutexIDs(A->isShared() ? SharedLocksAcquired
                                   : ExclusiveLocksAcquired,
                     A, nullptr, D);

test/SemaCXX/warn-thread-safety-analysis.cpp

public:
   void readerLock() SHARED_LOCK_FUNCTION(mu_) {
     mu_.ReaderLock();
   }
 
   void unlock() UNLOCK_FUNCTION(mu_) {
     mu_.Unlock();
   }
 
+  void unlockExclusive() EXCLUSIVE_UNLOCK_FUNCTION(mu_) {
+    mu_.Unlock();
+  }
+
+  void unlockShared() SHARED_UNLOCK_FUNCTION(mu_) {
+    mu_.ReaderUnlock();
+  }
+
   // Check failure to lock.
   void lockBad() EXCLUSIVE_LOCK_FUNCTION(mu_) {    // expected-note {{mutex acquired here}}
     mu2_.Lock();
     mu2_.Unlock();
   }  // expected-warning {{expecting mutex 'mu_' to be held at the end of function}}
 
   void readerLockBad() SHARED_LOCK_FUNCTION(mu_) {  // expected-note {{mutex acquired here}}
     mu2_.Lock();