[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349300: Thread safety analysis: Allow scoped releasing of 
capabilities (authored by aaronpuchert, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52578/new/

https://reviews.llvm.org/D52578

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

Index: cfe/trunk/lib/Analysis/ThreadSafety.cpp
===
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp
@@ -42,6 +42,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/ImmutableMap.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
@@ -890,28 +891,46 @@
 
 class ScopedLockableFactEntry : public FactEntry {
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.
+UCK_ReleasedShared,///< Shared capability that was released.
+UCK_ReleasedExclusive, ///< Exclusive capability that was released.
+  };
+
+  using UnderlyingCapability =
+  llvm::PointerIntPair;
+
+  SmallVector UnderlyingMutexes;
 
 public:
   ScopedLockableFactEntry(const CapabilityExpr , SourceLocation Loc,
-  const CapExprSet , const CapExprSet )
+  const CapExprSet , const CapExprSet ,
+  const CapExprSet , const CapExprSet )
   : FactEntry(CE, LK_Exclusive, Loc, false) {
 for (const auto  : Excl)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
 for (const auto  : Shrd)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
+for (const auto  : ExclRel)
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
+for (const auto  : ShrdRel)
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedShared);
   }
 
   void
   handleRemovalFromIntersection(const FactSet , FactManager ,
 SourceLocation JoinLoc, LockErrorKind LEK,
 ThreadSafetyHandler ) const override {
-for (const auto *UnderlyingMutex : UnderlyingMutexes) {
-  if (FSet.findLock(FactMan, CapabilityExpr(UnderlyingMutex, false))) {
+for (const auto  : UnderlyingMutexes) {
+  const auto *Entry = FSet.findLock(
+  FactMan, CapabilityExpr(UnderlyingMutex.getPointer(), false));
+  if ((UnderlyingMutex.getInt() == UCK_Acquired && Entry) ||
+  (UnderlyingMutex.getInt() != UCK_Acquired && !Entry)) {
 // If this scoped lock manages another mutex, and if the underlying
-// mutex is still held, then warn about the underlying mutex.
+// mutex is still/not held, then warn about the underlying mutex.
 Handler.handleMutexHeldEndOfScope(
-"mutex", sx::toString(UnderlyingMutex), loc(), JoinLoc, LEK);
+"mutex", sx::toString(UnderlyingMutex.getPointer()), loc(), JoinLoc,
+LEK);
   }
 }
   }
@@ -919,17 +938,14 @@
   void handleLock(FactSet , FactManager , const FactEntry ,
   ThreadSafetyHandler ,
   StringRef DiagKind) const override {
-for (const auto *UnderlyingMutex : UnderlyingMutexes) {
-  CapabilityExpr UnderCp(UnderlyingMutex, false);
+for (const auto  : UnderlyingMutexes) {
+  CapabilityExpr UnderCp(UnderlyingMutex.getPointer(), false);
 
-  // We're relocking the underlying mutexes. Warn on double locking.
-  if (FSet.findLock(FactMan, UnderCp)) {
-Handler.handleDoubleLock(DiagKind, UnderCp.toString(), entry.loc());
-  } else {
-FSet.removeLock(FactMan, !UnderCp);
-FSet.addLock(FactMan, llvm::make_unique(
-  UnderCp, entry.kind(), entry.loc()));
-  }
+  if (UnderlyingMutex.getInt() == UCK_Acquired)
+lock(FSet, FactMan, UnderCp, entry.kind(), entry.loc(), ,
+ DiagKind);
+  else
+unlock(FSet, FactMan, UnderCp, entry.loc(), , DiagKind);
 }
   }
 
@@ -938,32 +954,49 @@
 bool FullyRemove, ThreadSafetyHandler ,
 StringRef DiagKind) const override {
 assert(!Cp.negative() && "Managing object cannot be negative.");
-for (const auto *UnderlyingMutex : UnderlyingMutexes) {
-  CapabilityExpr UnderCp(UnderlyingMutex, false);
-  auto UnderEntry = llvm::make_unique(
-  !UnderCp, LK_Exclusive, UnlockLoc);
-
-  if (FullyRemove) {
-// We're destroying the managing object.
-// Remove the underlying mutex if it exists; but don't warn.
-if (FSet.findLock(FactMan, UnderCp)) 

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Delesley Hutchins via Phabricator via cfe-commits]

delesley accepted this revision.
delesley added a comment.

Just to be clear, I'm approving the change, but I'd still like the methods to 
be renamed.  :-)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52578/new/

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Victor Costan via Phabricator via cfe-commits]

pwnall added a comment.

In D52578#1307145 , @aaronpuchert 
wrote:

> To be clear, I'm not a big fan of this change myself, I just wanted to see if 
> it was feasible. My personal opinion, as I wrote in the bug report, is that 
> scoped releasing of mutexes is taking RAII a step too far. I'm putting this 
> on ice for now until we're reached a state where it looks a bit less crazy. I 
> hope @pwnall can live with that, since Clang 8 will not come out soon anyway.


If it makes a difference, Chrome (loosely) tracks Clang trunk 
.
 We wouldn't wait for a stable release to adopt the new code.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52578/new/

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

> As the analysis grew more complex, I switched to the current system based on 
> "facts". There are a number of facts that are potentially useful in static 
> analysis, such as whether one expression aliases another, and most of them 
> don't look at all like capabilities. IMHO, knowing whether an object is 
> within scope falls into this class.

Agreed. Though currently scoped capabilities aren't (necessarily) added/removed 
from the fact set on construction/destruction, only when the 
constructor/destructor is annotated as `ACQUIRE()`/`RELEASE()`. I was thinking 
about changing this, but haven't looked into it yet. This would clearly 
separate the fact of holding a scoped “capability” from the actual capabilities 
it represents.

> The only real argument for treating scoped lockable objects as proxies, which 
> can be "locked" and "unlocked", is that you can (in a future patch) reuse the 
> existing acquire_capability and release_capability attributes to support 
> releasing and then re-acquiring the proxy. It's a bit counter-intuitive, but 
> the alternative is adding new attributes, which is also bad.

Scoped capabilities can already be released (before destruction) since your 
change rC159387 , and I added the 
possibility to reacquire them in rC340459 . 
I agree that it's technically an abuse of notation 
, but one can wrap it's head 
around it after a while. It is not possible though to annotate functions 
outside of the scoped capability class as acquiring/releasing a scoped 
capability. Right now I don't see a reason to change that.

To be clear, I'm not a big fan of this change myself, I just wanted to see if 
it was feasible. My personal opinion, as I wrote in the bug report, is that 
scoped releasing of mutexes is taking RAII a step too far. I'm putting this on 
ice for now until we're reached a state where it looks a bit less crazy. I hope 
@pwnall can live with that, since Clang 8 will not come out soon anyway.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52578/new/

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added inline comments.



Comment at: lib/Analysis/ThreadSafety.cpp:951
+}
   } else {
+// We're removing the underlying mutex. Warn on double unlocking.

aaronpuchert wrote:
> aaronpuchert wrote:
> > delesley wrote:
> > > aaronpuchert wrote:
> > > > delesley wrote:
> > > > > I find this very confusing.  A lock here unlocks the underlying 
> > > > > mutex, and vice versa.  At the very least, some methods need to be 
> > > > > renamed, or maybe we can have separate classes for ScopedLockable and 
> > > > > ScopedUnlockable. 
> > > > I agree that it's confusing, but it follows what I think was the idea 
> > > > behind scoped capabilities: that they are also capabilities that can be 
> > > > acquired/released. Now since the scoped capability releases a mutex on 
> > > > construction (i.e. when it is acquired), it has to acquire the mutex 
> > > > when released. So the way it handles the released mutexes mirrors what 
> > > > happens on the scoped capability itself.
> > > > 
> > > > It's definitely not very intuitive, but I feel it's the most consistent 
> > > > variant with what we have already.
> > > > 
> > > > The nice thing about this is that it works pretty well with the 
> > > > existing semantics and allows constructs such as `MutexLockUnlock` (see 
> > > > the tests) that unlocks one mutex and locks another. Not sure if anyone 
> > > > needs this, but why not?
> > > A scoped_lockable object is not a capability, it is an object that 
> > > manages capabilities.  IMHO, conflating those two concepts is a bad idea, 
> > > and recipe for confusion.  You would not, for example, use a 
> > > scoped_lockable object in a guarded_by attribute.  
> > > 
> > > Unfortunately, the existing code tends to conflate "capabilities" and 
> > > "facts", which is my fault.  A scoped_lockable object is a valid fact -- 
> > > the fact records whether the object is in scope -- it's just not a valid 
> > > capability. 
> > > 
> > > Simply renaming the methods from handleLock/Unlock to  addFact/removeFact 
> > > would be a nice first step in making the code clearer, and would 
> > > distinguish between facts that refer to capabilities, and facts that 
> > > refer to scoped objects.  
> > > 
> > > 
> > > 
> > > 
> > Makes sense to me. I'll see if I can refactor some of the code to clarify 
> > this.
> > 
> > I think that we should also separate between unlocking the underlying 
> > mutexes and destructing the scoped_lockable, which is currently handled via 
> > the extra parameter `FullyRemove` of `handleUnlock`.
> Scoped capabilities are not the same as capabilities, but they are also more 
> than just facts. (Except if you follow 
> [Wittgenstein](https://en.wikipedia.org/wiki/Tractatus_Logico-Philosophicus#Proposition_1),
>  perhaps.) They can be released, and reacquired, which makes them more than 
> mere immutable facts. I think we can consider them as proxy capabilities. 
> They cannot directly protect any resource, but we can acquire and release 
> (actual) capabilities through them. That's why it doesn't sound crazy to me 
> to say that releasing a scoped capability might acquire an underlying mutex, 
> if that was released on construction of the scoped capability.
The notion of "proxy" is how scoped objects have always been treated in the 
past, but that's mostly a historical accident, rather than a well-reasoned 
design choice.  As a bit of history, the first version of thread safety 
analysis only tracked mutexes.  Thus, the original implementation pretended 
that a scoped object was a mutex, because that's the only thing that the 
analysis could deal with.

As the analysis grew more complex, I switched to the current system based on 
"facts".  There are a number of facts that are potentially useful in static 
analysis, such as whether one expression aliases another, and most of them 
don't look at all like capabilities.  IMHO, knowing whether an object is within 
scope falls into this class.  The basic feature of facts is that they are 
control flow dependent -- they can be added to sets, removed from sets, and 
removed from intersections at joint points in the CFG.  Renaming the methods to 
reflect this design will make future extensions easier to understand.

The only real argument for treating scoped lockable objects as proxies, which 
can be "locked" and "unlocked", is that you can (in a future patch) reuse the 
existing acquire_capability and release_capability attributes to support 
releasing and then re-acquiring the proxy.  It's a bit counter-intuitive, but 
the alternative is adding new attributes, which is also bad.

However, even if you reuse the existing attributes, when it comes time to 
define a ScopedUnlockable class, you *still* shouldn't provide methods named 
"lock" and "unlock", where "lock"  releases the underlying mutex, and "unlock" 
reacquires it.  That's bad API design which will be endlessly confusing to 
users.

Thus, I would still 

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 171026.
aaronpuchert added a comment.

Negative capabilities don't need a LockKind.


Repository:
  rC Clang

https://reviews.llvm.org/D52578

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
@@ -2787,6 +2787,110 @@
 } // end namespace RelockableScopedLock
 
 
+namespace ScopedUnlock {
+
+class SCOPED_LOCKABLE MutexUnlock {
+public:
+  MutexUnlock(Mutex *mu) EXCLUSIVE_UNLOCK_FUNCTION(mu);
+  ~MutexUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Unlock() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE ReaderMutexUnlock {
+public:
+  ReaderMutexUnlock(Mutex *mu) SHARED_UNLOCK_FUNCTION(mu);
+  ~ReaderMutexUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Unlock() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+bool c;
+void print(int);
+
+void simple() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  x = 1;
+  MutexUnlock scope();
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void simpleShared() SHARED_LOCKS_REQUIRED(mu) {
+  print(x);
+  ReaderMutexUnlock scope();
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+}
+
+void innerUnlock() {
+  MutexLock outer();
+  if (x == 0) {
+MutexUnlock inner();
+x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  }
+  x = 2;
+}
+
+void innerUnlockShared() {
+  ReaderMutexLock outer();
+  if (x == 0) {
+ReaderMutexUnlock inner();
+print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  }
+  print(x);
+}
+
+void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Lock();
+  x = 2;
+  scope.Unlock();
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void join() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  if (c) {
+scope.Lock(); // expected-note{{mutex acquired here}}
+  }
+  // expected-warning@+1{{mutex 'mu' is not held on every path through here}}
+  scope.Lock();
+}
+
+void doubleLock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+class SCOPED_LOCKABLE MutexLockUnlock {
+public:
+  MutexLockUnlock(Mutex *mu1, Mutex *mu2) EXCLUSIVE_UNLOCK_FUNCTION(mu1) EXCLUSIVE_LOCK_FUNCTION(mu2);
+  ~MutexLockUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Release() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Acquire() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex other;
+void fn() EXCLUSIVE_LOCKS_REQUIRED(other);
+
+void lockUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexLockUnlock scope(, );
+  fn();
+  x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+} // end namespace ScopedUnlock
+
+
 namespace TrylockFunctionTest {
 
 class Foo {
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -42,6 +42,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/ImmutableMap.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
@@ -890,80 +891,112 @@
 
 class ScopedLockableFactEntry : public FactEntry {
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.
+UCK_ReleasedShared,///< Shared capability that was released.
+UCK_ReleasedExclusive, ///< Exclusive capability that was released.
+  };
+
+  using UnderlyingCapability =
+  llvm::PointerIntPair;
+
+  SmallVector UnderlyingMutexes;
 
 public:
   ScopedLockableFactEntry(const CapabilityExpr , SourceLocation Loc,
-  const CapExprSet , const CapExprSet )
+  const CapExprSet , const CapExprSet ,
+  const CapExprSet , const CapExprSet )
   : FactEntry(CE, LK_Exclusive, Loc, false) {
 for (const auto  : Excl)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
 for (const auto  : Shrd)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
+for (const auto  : ExclRel)
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 171019.
aaronpuchert added a comment.

Introduced helper functions to clarify lock handling.

The previous version was too tightly coupled, and the introduction of AddCp and 
RemoveCp didn't help readability.


Repository:
  rC Clang

https://reviews.llvm.org/D52578

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
@@ -2787,6 +2787,110 @@
 } // end namespace RelockableScopedLock
 
 
+namespace ScopedUnlock {
+
+class SCOPED_LOCKABLE MutexUnlock {
+public:
+  MutexUnlock(Mutex *mu) EXCLUSIVE_UNLOCK_FUNCTION(mu);
+  ~MutexUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Unlock() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE ReaderMutexUnlock {
+public:
+  ReaderMutexUnlock(Mutex *mu) SHARED_UNLOCK_FUNCTION(mu);
+  ~ReaderMutexUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Unlock() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+bool c;
+void print(int);
+
+void simple() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  x = 1;
+  MutexUnlock scope();
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void simpleShared() SHARED_LOCKS_REQUIRED(mu) {
+  print(x);
+  ReaderMutexUnlock scope();
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+}
+
+void innerUnlock() {
+  MutexLock outer();
+  if (x == 0) {
+MutexUnlock inner();
+x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  }
+  x = 2;
+}
+
+void innerUnlockShared() {
+  ReaderMutexLock outer();
+  if (x == 0) {
+ReaderMutexUnlock inner();
+print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  }
+  print(x);
+}
+
+void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Lock();
+  x = 2;
+  scope.Unlock();
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void join() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  if (c) {
+scope.Lock(); // expected-note{{mutex acquired here}}
+  }
+  // expected-warning@+1{{mutex 'mu' is not held on every path through here}}
+  scope.Lock();
+}
+
+void doubleLock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+class SCOPED_LOCKABLE MutexLockUnlock {
+public:
+  MutexLockUnlock(Mutex *mu1, Mutex *mu2) EXCLUSIVE_UNLOCK_FUNCTION(mu1) EXCLUSIVE_LOCK_FUNCTION(mu2);
+  ~MutexLockUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Release() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Acquire() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex other;
+void fn() EXCLUSIVE_LOCKS_REQUIRED(other);
+
+void lockUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexLockUnlock scope(, );
+  fn();
+  x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+} // end namespace ScopedUnlock
+
+
 namespace TrylockFunctionTest {
 
 class Foo {
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -42,6 +42,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/ImmutableMap.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
@@ -890,80 +891,115 @@
 
 class ScopedLockableFactEntry : public FactEntry {
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.
+UCK_ReleasedShared,///< Shared capability that was released.
+UCK_ReleasedExclusive, ///< Exclusive capability that was released.
+  };
+
+  using UnderlyingCapability =
+  llvm::PointerIntPair;
+
+  SmallVector UnderlyingMutexes;
 
 public:
   ScopedLockableFactEntry(const CapabilityExpr , SourceLocation Loc,
-  const CapExprSet , const CapExprSet )
+  const CapExprSet , const CapExprSet ,
+  const CapExprSet , const CapExprSet )
   : FactEntry(CE, LK_Exclusive, Loc, false) {
 for (const auto  : Excl)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
 for (const auto  : Shrd)
-  UnderlyingMutexes.push_back(M.sexpr());
+  

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

I hope the cleanup makes the code more easily digestible, and to some extent 
might also transport why I think this is the most elegant approach.

I think we should document the semantics of scoped capabilities in more detail, 
and I will do so once this is either merged, or we decided that we don't want 
to merge it.




Comment at: lib/Analysis/ThreadSafety.cpp:893
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.

aaronpuchert wrote:
> delesley wrote:
> > aaronpuchert wrote:
> > > delesley wrote:
> > > > IMHO, it would make more sense to break this into two properties (or 
> > > > bits): acquired/released and shared/exclusive. 
> > > We don't use the information (shared/exclusive) for acquired locks, but 
> > > we need two bits anyway, so why not.
> > The normal unlock doesn't distinguish between shared/exclusive, but there 
> > is a release_shared_capability which does...
> Right, but see [bug 33504](https://bugs.llvm.org/show_bug.cgi?id=33504): 
> currently `release_shared_capability` does not work with scoped capabilities. 
> If we allow it, we would have to discuss when it is allowed. A scoped 
> capability can lock multiple locks, some of them shared, some of them 
> exclusive.
> 
> My assumption, as I wrote in the bug: scoped capabilities are always 
> exclusive, hence can only be unlocked exclusively, but automatically release 
> underlying mutexes in the right mode.
I've thought about splitting up `UCK_Acquired` into a shared and exclusive 
variant, but not done it. Here's why: we don't need to know how the mutex was 
initially acquired for releasing it, and for re-acquisition we use the 
attribute on the "relock" method of the scoped capability. With released 
capabilities however we need to know in which mode they should be acquired 
again. Another reason is that we might want a fourth enumeration value to 
implement something in the direction of 
[`std::defer_lock_t`](https://en.cppreference.com/w/cpp/thread/unique_lock/unique_lock),
 and we might only have two bits.



Comment at: lib/Analysis/ThreadSafety.cpp:951
+}
   } else {
+// We're removing the underlying mutex. Warn on double unlocking.

aaronpuchert wrote:
> delesley wrote:
> > aaronpuchert wrote:
> > > delesley wrote:
> > > > I find this very confusing.  A lock here unlocks the underlying mutex, 
> > > > and vice versa.  At the very least, some methods need to be renamed, or 
> > > > maybe we can have separate classes for ScopedLockable and 
> > > > ScopedUnlockable. 
> > > I agree that it's confusing, but it follows what I think was the idea 
> > > behind scoped capabilities: that they are also capabilities that can be 
> > > acquired/released. Now since the scoped capability releases a mutex on 
> > > construction (i.e. when it is acquired), it has to acquire the mutex when 
> > > released. So the way it handles the released mutexes mirrors what happens 
> > > on the scoped capability itself.
> > > 
> > > It's definitely not very intuitive, but I feel it's the most consistent 
> > > variant with what we have already.
> > > 
> > > The nice thing about this is that it works pretty well with the existing 
> > > semantics and allows constructs such as `MutexLockUnlock` (see the tests) 
> > > that unlocks one mutex and locks another. Not sure if anyone needs this, 
> > > but why not?
> > A scoped_lockable object is not a capability, it is an object that manages 
> > capabilities.  IMHO, conflating those two concepts is a bad idea, and 
> > recipe for confusion.  You would not, for example, use a scoped_lockable 
> > object in a guarded_by attribute.  
> > 
> > Unfortunately, the existing code tends to conflate "capabilities" and 
> > "facts", which is my fault.  A scoped_lockable object is a valid fact -- 
> > the fact records whether the object is in scope -- it's just not a valid 
> > capability. 
> > 
> > Simply renaming the methods from handleLock/Unlock to  addFact/removeFact 
> > would be a nice first step in making the code clearer, and would 
> > distinguish between facts that refer to capabilities, and facts that refer 
> > to scoped objects.  
> > 
> > 
> > 
> > 
> Makes sense to me. I'll see if I can refactor some of the code to clarify 
> this.
> 
> I think that we should also separate between unlocking the underlying mutexes 
> and destructing the scoped_lockable, which is currently handled via the extra 
> parameter `FullyRemove` of `handleUnlock`.
Scoped capabilities are not the same as capabilities, but they are also more 
than just facts. (Except if you follow 
[Wittgenstein](https://en.wikipedia.org/wiki/Tractatus_Logico-Philosophicus#Proposition_1),
 perhaps.) They can be released, and reacquired, which makes them more than 
mere immutable facts. I think we can consider them as proxy capabilities. They 

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 170545.
aaronpuchert marked 3 inline comments as done.
aaronpuchert added a comment.
This revision is now accepted and ready to land.

Addressed some review comments and simplified the code.

There is a lot less duplication and maybe it's even easier to understand.


Repository:
  rC Clang

https://reviews.llvm.org/D52578

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
@@ -2787,6 +2787,110 @@
 } // end namespace RelockableScopedLock
 
 
+namespace ScopedUnlock {
+
+class SCOPED_LOCKABLE MutexUnlock {
+public:
+  MutexUnlock(Mutex *mu) EXCLUSIVE_UNLOCK_FUNCTION(mu);
+  ~MutexUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Unlock() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE ReaderMutexUnlock {
+public:
+  ReaderMutexUnlock(Mutex *mu) SHARED_UNLOCK_FUNCTION(mu);
+  ~ReaderMutexUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Unlock() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+bool c;
+void print(int);
+
+void simple() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  x = 1;
+  MutexUnlock scope();
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void simpleShared() SHARED_LOCKS_REQUIRED(mu) {
+  print(x);
+  ReaderMutexUnlock scope();
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+}
+
+void innerUnlock() {
+  MutexLock outer();
+  if (x == 0) {
+MutexUnlock inner();
+x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  }
+  x = 2;
+}
+
+void innerUnlockShared() {
+  ReaderMutexLock outer();
+  if (x == 0) {
+ReaderMutexUnlock inner();
+print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  }
+  print(x);
+}
+
+void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Lock();
+  x = 2;
+  scope.Unlock();
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void join() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  if (c) {
+scope.Lock(); // expected-note{{mutex acquired here}}
+  }
+  // expected-warning@+1{{mutex 'mu' is not held on every path through here}}
+  scope.Lock();
+}
+
+void doubleLock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+class SCOPED_LOCKABLE MutexLockUnlock {
+public:
+  MutexLockUnlock(Mutex *mu1, Mutex *mu2) EXCLUSIVE_UNLOCK_FUNCTION(mu1) EXCLUSIVE_LOCK_FUNCTION(mu2);
+  ~MutexLockUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Release() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Acquire() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex other;
+void fn() EXCLUSIVE_LOCKS_REQUIRED(other);
+
+void lockUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexLockUnlock scope(, );
+  fn();
+  x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+} // end namespace ScopedUnlock
+
+
 namespace TrylockFunctionTest {
 
 class Foo {
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -42,6 +42,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/ImmutableMap.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
@@ -890,45 +891,81 @@
 
 class ScopedLockableFactEntry : public FactEntry {
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.
+UCK_ReleasedShared,///< Shared capability that was released.
+UCK_ReleasedExclusive, ///< Exclusive capability that was released.
+  };
+
+  static LockKind getUnlockKind(UnderlyingCapabilityKind kind) {
+switch (kind) {
+case UCK_Acquired:
+  return LK_Exclusive;
+case UCK_ReleasedShared:
+  return LK_Shared;
+case UCK_ReleasedExclusive:
+  return LK_Exclusive;
+}
+llvm_unreachable("Unknown UnderlyingCapabilityKind");
+  }
+
+  using UnderlyingCapability =
+  llvm::PointerIntPair;
+
+  SmallVector UnderlyingMutexes;
 
 public:
   ScopedLockableFactEntry(const CapabilityExpr , SourceLocation Loc,
-  const CapExprSet , const CapExprSet )
+  const CapExprSet , const 

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Victor Costan via Phabricator via cfe-commits]

pwnall added a comment.

Thank you for clarifying, Aaron!

I probably used Phabricator incorrectly. My intent was to state that the tests 
LGTM and express support for the functionality in this patch. Please definitely 
address all review comments before landing.


Repository:
  rC Clang

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert planned changes to this revision.
aaronpuchert added a comment.

I think I'll try to simplify this and address @delesley's comments before we 
commit this. I'll admit that the semantics are somewhat counter-intuitive, but 
as I explained I think it's more consistent this way. Because the scoped unlock 
releases a lock on construction, the operation on the underlying lock mirrors 
the operation on the scoped capability: releasing the scoped capability (on 
destruction, for example) acquires the lock again.


Repository:
  rC Clang

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Victor Costan via Phabricator via cfe-commits]

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

test/SemaCXX/warn-thread-safety-analysis.cpp LGTM -- this is the functionality 
that we need in Chrome to use thread safety annotations for AutoUnlock.

very non-authoritative opinion - I think the tests would be a bit easier to 
follow if Lock() would be the EXCLUSIVE_LOCK_FUNCTION and Unlock() would be the 
EXCLUSIVE_UNLOCK_FUNCTION.

aaronpuchert: Thank you for the patch! I'd be happy and grateful to have this 
functionality in Chrome.
delesley: Thank you for your review and guidance!


Repository:
  rC Clang

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: lib/Analysis/ThreadSafety.cpp:893
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.

delesley wrote:
> aaronpuchert wrote:
> > delesley wrote:
> > > IMHO, it would make more sense to break this into two properties (or 
> > > bits): acquired/released and shared/exclusive. 
> > We don't use the information (shared/exclusive) for acquired locks, but we 
> > need two bits anyway, so why not.
> The normal unlock doesn't distinguish between shared/exclusive, but there is 
> a release_shared_capability which does...
Right, but see [bug 33504](https://bugs.llvm.org/show_bug.cgi?id=33504): 
currently `release_shared_capability` does not work with scoped capabilities. 
If we allow it, we would have to discuss when it is allowed. A scoped 
capability can lock multiple locks, some of them shared, some of them exclusive.

My assumption, as I wrote in the bug: scoped capabilities are always exclusive, 
hence can only be unlocked exclusively, but automatically release underlying 
mutexes in the right mode.



Comment at: lib/Analysis/ThreadSafety.cpp:951
+}
   } else {
+// We're removing the underlying mutex. Warn on double unlocking.

delesley wrote:
> aaronpuchert wrote:
> > delesley wrote:
> > > I find this very confusing.  A lock here unlocks the underlying mutex, 
> > > and vice versa.  At the very least, some methods need to be renamed, or 
> > > maybe we can have separate classes for ScopedLockable and 
> > > ScopedUnlockable. 
> > I agree that it's confusing, but it follows what I think was the idea 
> > behind scoped capabilities: that they are also capabilities that can be 
> > acquired/released. Now since the scoped capability releases a mutex on 
> > construction (i.e. when it is acquired), it has to acquire the mutex when 
> > released. So the way it handles the released mutexes mirrors what happens 
> > on the scoped capability itself.
> > 
> > It's definitely not very intuitive, but I feel it's the most consistent 
> > variant with what we have already.
> > 
> > The nice thing about this is that it works pretty well with the existing 
> > semantics and allows constructs such as `MutexLockUnlock` (see the tests) 
> > that unlocks one mutex and locks another. Not sure if anyone needs this, 
> > but why not?
> A scoped_lockable object is not a capability, it is an object that manages 
> capabilities.  IMHO, conflating those two concepts is a bad idea, and recipe 
> for confusion.  You would not, for example, use a scoped_lockable object in a 
> guarded_by attribute.  
> 
> Unfortunately, the existing code tends to conflate "capabilities" and 
> "facts", which is my fault.  A scoped_lockable object is a valid fact -- the 
> fact records whether the object is in scope -- it's just not a valid 
> capability. 
> 
> Simply renaming the methods from handleLock/Unlock to  addFact/removeFact 
> would be a nice first step in making the code clearer, and would distinguish 
> between facts that refer to capabilities, and facts that refer to scoped 
> objects.  
> 
> 
> 
> 
Makes sense to me. I'll see if I can refactor some of the code to clarify this.

I think that we should also separate between unlocking the underlying mutexes 
and destructing the scoped_lockable, which is currently handled via the extra 
parameter `FullyRemove` of `handleUnlock`.



Comment at: lib/Analysis/ThreadSafety.cpp:992
+FSet.removeLock(FactMan, UnderCp);
+FSet.addLock(FactMan, std::move(UnderEntry));
+  }

delesley wrote:
> aaronpuchert wrote:
> > delesley wrote:
> > > Shouldn't this be outside of the else?
> > If we don't find `UnderCp` anymore, the negation should be there already. 
> > But I can also keep it outside if you like.
> That's not necessarily true.  Not knowing whether a mutex is locked is 
> different from knowing that it is unlocked. 
> 
> You're probably right in this case, because capabilities that are managed by 
> scoped objects obey a more restricted set of rules.  But then, you're also 
> extending the ways in which scoped objects can be used.  :-)
> 
Emphasis is on "anymore". We know we held `UnderCp` once (on construction) and 
it has been released since then. Since we have to add the negative capability 
whenever a lock is removed, it must already be there.

If we introduce support for deferred locking (like `std::defer_lock_t`) we 
would have to make sure we add the negative capability right at the beginning.


Repository:
  rC Clang

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added inline comments.



Comment at: lib/Analysis/ThreadSafety.cpp:893
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.

aaronpuchert wrote:
> delesley wrote:
> > IMHO, it would make more sense to break this into two properties (or bits): 
> > acquired/released and shared/exclusive. 
> We don't use the information (shared/exclusive) for acquired locks, but we 
> need two bits anyway, so why not.
The normal unlock doesn't distinguish between shared/exclusive, but there is a 
release_shared_capability which does...


Repository:
  rC Clang

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added inline comments.



Comment at: lib/Analysis/ThreadSafety.cpp:951
+}
   } else {
+// We're removing the underlying mutex. Warn on double unlocking.

aaronpuchert wrote:
> delesley wrote:
> > I find this very confusing.  A lock here unlocks the underlying mutex, and 
> > vice versa.  At the very least, some methods need to be renamed, or maybe 
> > we can have separate classes for ScopedLockable and ScopedUnlockable. 
> I agree that it's confusing, but it follows what I think was the idea behind 
> scoped capabilities: that they are also capabilities that can be 
> acquired/released. Now since the scoped capability releases a mutex on 
> construction (i.e. when it is acquired), it has to acquire the mutex when 
> released. So the way it handles the released mutexes mirrors what happens on 
> the scoped capability itself.
> 
> It's definitely not very intuitive, but I feel it's the most consistent 
> variant with what we have already.
> 
> The nice thing about this is that it works pretty well with the existing 
> semantics and allows constructs such as `MutexLockUnlock` (see the tests) 
> that unlocks one mutex and locks another. Not sure if anyone needs this, but 
> why not?
A scoped_lockable object is not a capability, it is an object that manages 
capabilities.  IMHO, conflating those two concepts is a bad idea, and recipe 
for confusion.  You would not, for example, use a scoped_lockable object in a 
guarded_by attribute.  

Unfortunately, the existing code tends to conflate "capabilities" and "facts", 
which is my fault.  A scoped_lockable object is a valid fact -- the fact 
records whether the object is in scope -- it's just not a valid capability. 

Simply renaming the methods from handleLock/Unlock to  addFact/removeFact would 
be a nice first step in making the code clearer, and would distinguish between 
facts that refer to capabilities, and facts that refer to scoped objects.  







Comment at: lib/Analysis/ThreadSafety.cpp:992
+FSet.removeLock(FactMan, UnderCp);
+FSet.addLock(FactMan, std::move(UnderEntry));
+  }

aaronpuchert wrote:
> delesley wrote:
> > Shouldn't this be outside of the else?
> If we don't find `UnderCp` anymore, the negation should be there already. But 
> I can also keep it outside if you like.
That's not necessarily true.  Not knowing whether a mutex is locked is 
different from knowing that it is unlocked. 

You're probably right in this case, because capabilities that are managed by 
scoped objects obey a more restricted set of rules.  But then, you're also 
extending the ways in which scoped objects can be used.  :-)



Repository:
  rC Clang

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Maybe you should have a look at the tests first. I thought about the semantics 
that I think you are suggesting, but then we could have code like:

  class SCOPED_LOCKABLE MutexLockUnlock {
  public:
MutexLockUnlock(Mutex *mu1, Mutex *mu2) EXCLUSIVE_UNLOCK_FUNCTION(mu1) 
EXCLUSIVE_LOCK_FUNCTION(mu2);
~MutexLockUnlock() /* what annotation do we use here? */;
  
void a() EXCLUSIVE_UNLOCK_FUNCTION();
void b() EXCLUSIVE_LOCK_FUNCTION();
  };

Then what do `a` and `b` do? Or do we not allow this pattern? It's not 
straightforward either way, which is why I wanted to talk my way out of this in 
the bug report. ;)




Comment at: lib/Analysis/ThreadSafety.cpp:893
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.

delesley wrote:
> IMHO, it would make more sense to break this into two properties (or bits): 
> acquired/released and shared/exclusive. 
We don't use the information (shared/exclusive) for acquired locks, but we need 
two bits anyway, so why not.



Comment at: lib/Analysis/ThreadSafety.cpp:916
+for (const auto  : ShrdRel)
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
   }

delesley wrote:
> UCK_ReleasedShared?  (Shouldn't this have been caught by a test case?)
There is no test case for the shared variant yet. I'll add one.



Comment at: lib/Analysis/ThreadSafety.cpp:951
+}
   } else {
+// We're removing the underlying mutex. Warn on double unlocking.

delesley wrote:
> I find this very confusing.  A lock here unlocks the underlying mutex, and 
> vice versa.  At the very least, some methods need to be renamed, or maybe we 
> can have separate classes for ScopedLockable and ScopedUnlockable. 
I agree that it's confusing, but it follows what I think was the idea behind 
scoped capabilities: that they are also capabilities that can be 
acquired/released. Now since the scoped capability releases a mutex on 
construction (i.e. when it is acquired), it has to acquire the mutex when 
released. So the way it handles the released mutexes mirrors what happens on 
the scoped capability itself.

It's definitely not very intuitive, but I feel it's the most consistent variant 
with what we have already.

The nice thing about this is that it works pretty well with the existing 
semantics and allows constructs such as `MutexLockUnlock` (see the tests) that 
unlocks one mutex and locks another. Not sure if anyone needs this, but why not?



Comment at: lib/Analysis/ThreadSafety.cpp:992
+FSet.removeLock(FactMan, UnderCp);
+FSet.addLock(FactMan, std::move(UnderEntry));
+  }

delesley wrote:
> Shouldn't this be outside of the else?
If we don't find `UnderCp` anymore, the negation should be there already. But I 
can also keep it outside if you like.


Repository:
  rC Clang

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added inline comments.



Comment at: lib/Analysis/ThreadSafety.cpp:893
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.

IMHO, it would make more sense to break this into two properties (or bits): 
acquired/released and shared/exclusive. 



Comment at: lib/Analysis/ThreadSafety.cpp:916
+for (const auto  : ShrdRel)
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
   }

UCK_ReleasedShared?  (Shouldn't this have been caught by a test case?)



Comment at: lib/Analysis/ThreadSafety.cpp:926
+  FactMan, CapabilityExpr(UnderlyingMutex.getPointer(), false));
+  if ((UnderlyingMutex.getInt() == UCK_Acquired) == (Entry != nullptr)) {
 // If this scoped lock manages another mutex, and if the underlying

This if statement makes my head hurt.  Can you find a different way of 
expressing it?



Comment at: lib/Analysis/ThreadSafety.cpp:951
+}
   } else {
+// We're removing the underlying mutex. Warn on double unlocking.

I find this very confusing.  A lock here unlocks the underlying mutex, and vice 
versa.  At the very least, some methods need to be renamed, or maybe we can 
have separate classes for ScopedLockable and ScopedUnlockable. 



Comment at: lib/Analysis/ThreadSafety.cpp:992
+FSet.removeLock(FactMan, UnderCp);
+FSet.addLock(FactMan, std::move(UnderEntry));
+  }

Shouldn't this be outside of the else?


Repository:
  rC Clang

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

See the bug for further discussion. I'm not sure if we want to have this, but 
if the pattern is used more widely it might make sense. It blows up the code a 
bit, although I hope that https://reviews.llvm.org/D51187 might reduce it again.


Repository:
  rC Clang

https://reviews.llvm.org/D52578



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

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

The pattern is problematic with C++ exceptions, and not as widespread as
scoped locks, but it's still used by some, for example Chromium.

We are a bit stricter here at join points, patterns that are allowed for
scoped locks aren't allowed here. That could still be changed in the
future, but I'd argue we should only relax this if people ask for it.

Fixes PR36162.


Repository:
  rC Clang

https://reviews.llvm.org/D52578

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
@@ -2755,6 +2755,85 @@
 } // end namespace RelockableScopedLock
 
 
+namespace ScopedUnlock {
+
+class SCOPED_LOCKABLE MutexUnlock {
+public:
+  MutexUnlock(Mutex *mu) EXCLUSIVE_UNLOCK_FUNCTION(mu);
+  ~MutexUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Unlock() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+bool c;
+
+void simple() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  x = 1;
+  MutexUnlock scope();
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void innerUnlock() {
+  MutexLock outer();
+  if (x == 0) {
+MutexUnlock inner();
+x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  }
+  x = 2;
+}
+
+void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Lock();
+  x = 2;
+  scope.Unlock();
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void join() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  if (c) {
+scope.Lock(); // expected-note{{mutex acquired here}}
+  }
+  // expected-warning@+1{{mutex 'mu' is not held on every path through here}}
+  scope.Lock();
+}
+
+void doubleLock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+class SCOPED_LOCKABLE MutexLockUnlock {
+public:
+  MutexLockUnlock(Mutex *mu1, Mutex *mu2) EXCLUSIVE_UNLOCK_FUNCTION(mu1) EXCLUSIVE_LOCK_FUNCTION(mu2);
+  ~MutexLockUnlock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Release() EXCLUSIVE_UNLOCK_FUNCTION();
+  void Acquire() EXCLUSIVE_LOCK_FUNCTION();
+};
+
+Mutex other;
+void fn() EXCLUSIVE_LOCKS_REQUIRED(other);
+
+void lockUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexLockUnlock scope(, );
+  fn();
+  x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+} // end namespace ScopedUnlock
+
+
 namespace TrylockFunctionTest {
 
 class Foo {
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -41,6 +41,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/ImmutableMap.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
@@ -889,45 +890,74 @@
 
 class ScopedLockableFactEntry : public FactEntry {
 private:
-  SmallVector UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+UCK_Acquired,  ///< Any kind of acquired capability.
+UCK_ReleasedShared,///< Shared capability that was released.
+UCK_ReleasedExclusive, ///< Exclusive capability that was released.
+  };
+
+  using UnderlyingCapability =
+  llvm::PointerIntPair;
+
+  SmallVector UnderlyingMutexes;
 
 public:
   ScopedLockableFactEntry(const CapabilityExpr , SourceLocation Loc,
-  const CapExprSet , const CapExprSet )
+  const CapExprSet , const CapExprSet ,
+  const CapExprSet , const CapExprSet )
   : FactEntry(CE, LK_Exclusive, Loc, false) {
 for (const auto  : Excl)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
 for (const auto  : Shrd)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
+for (const auto  : ExclRel)
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
+for (const auto  : ShrdRel)
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
   }
 
   void
   handleRemovalFromIntersection(const FactSet , FactManager ,
 SourceLocation JoinLoc, LockErrorKind LEK,