[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] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked an inline comment as done.
aaronpuchert added a comment.

@delesley Did you have a chance to look at this yet?


Repository:
  rC Clang

https://reviews.llvm.org/D49885



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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

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

This didn't cross my mind, because an `ACQUIRES` attribute with arguments on a 
function other than the constructor does not add the argument locks to the set 
of managed mutexes. So while I'm not sure why someone would do that, it's 
completely possible without warning. Indeed it would be possible to have 
`ACQUIRES` both with and without arguments on the same function.


Repository:
  rC Clang

https://reviews.llvm.org/D49885



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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

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

Fix crash. The problem was that ACQUIRES with arguments did not no longer have 
(only) `this` as argument, hence our assumption that we would have only 
ScopedLockableFactEntry's was false. So now we lock a bit closer which kind of 
ACQUIRES we actually have.


Repository:
  rC Clang

https://reviews.llvm.org/D49885

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
@@ -2621,6 +2621,196 @@
 } // end namespace ReleasableScopedLock
 
 
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+  RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE RelockableSharedMutexLock {
+public:
+  RelockableSharedMutexLock(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+  ~RelockableSharedMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() SHARED_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+class SharedTraits {};
+class ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+  RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+  RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void ReaderUnlock() UNLOCK_FUNCTION();
+
+  void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void write() {
+  RelockableExclusiveMutexLock scope(&mu);
+  x = 2;
+  scope.Unlock();
+
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  x = 4;
+}
+
+void read() {
+  RelockableSharedMutexLock scope(&mu);
+  print(x);
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockExclusive() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.ReaderUnlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.Lock();
+  print(x);
+  x = 4;
+
+  scope.DemoteExclusive();
+  print(x);
+  x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{});
+  print(x);
+  x = 2;
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.ReaderLock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.PromoteShared();
+  print(x);
+  x = 5;
+}
+
+void doubleUnlock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleUnlock2() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock3() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock4() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.~RelockableExclusiveMutexLock();
+  scope.Lock(); // Maybe this should warn.
+} // expected-warning {{releasing mutex 'scope' that was not held}}
+
+class SCOPED_LOCKABLE MemberLock {
+ public:
+  MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  ~MemberLock() UNLOCK_FUNCTION(mutex);
+  void Lo

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

The case distinction in `case attr::AcquireCapability` is not very beautiful, 
but it's due to the fact that scoped capabilities are not "real" capabilities 
and so we need to distinguish them.

What this still doesn't allow for is attributes on other classes than the 
scoped capability that reacquire it from the outside. So maybe this isn't the 
right solution, and we need to approach it in a different way. Maybe instead of 
modifying the `BuildLockset::handleCall`, we should change 
`ThreadSafetyAnalyzer::addLock`. I'll think about that, but not today.




Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:2769-2781
+class SCOPED_LOCKABLE MemberLock {
+ public:
+  MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  ~MemberLock() UNLOCK_FUNCTION(mutex);
+  void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  Mutex mutex;
+};

@hokein This is your test case, I just renamed some things.


Repository:
  rC Clang

https://reviews.llvm.org/D49885



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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

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

Found a much simpler solution. After introducing a new virtual function 
HandleLock() in FactEntry, I just needed to change two lines in 
ThreadSafetyAnalyzer::addLock. Changes in BuildLockset::handleCall are no 
longer necessary.


Repository:
  rC Clang

https://reviews.llvm.org/D49885

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
@@ -2621,6 +2621,213 @@
 } // end namespace ReleasableScopedLock
 
 
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+  RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE RelockableSharedMutexLock {
+public:
+  RelockableSharedMutexLock(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+  ~RelockableSharedMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() SHARED_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+class SharedTraits {};
+class ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+  RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+  RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void ReaderUnlock() UNLOCK_FUNCTION();
+
+  void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void write() {
+  RelockableExclusiveMutexLock scope(&mu);
+  x = 2;
+  scope.Unlock();
+
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  x = 4;
+}
+
+void read() {
+  RelockableSharedMutexLock scope(&mu);
+  print(x);
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockExclusive() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.ReaderUnlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.Lock();
+  print(x);
+  x = 4;
+
+  scope.DemoteExclusive();
+  print(x);
+  x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{});
+  print(x);
+  x = 2;
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.ReaderLock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.PromoteShared();
+  print(x);
+  x = 5;
+}
+
+void doubleUnlock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleUnlock2() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock3() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock4() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  mu.Unlock();
+  // Debatable that there is no warning. Currently we don't track in the scoped
+  // object whether it is active, but just check if the contained locks can be
+  // reacquired. Here they can, because mu has been unlocked manually.
+  scope.Lock();
+}
+
+void directRelock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  mu.Lock();
+  // Similarly debatable tha

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked 4 inline comments as done.
aaronpuchert added a comment.

@aaron.ballman Maybe you can have a look again — this is much more elegant. I'm 
not sure why I didn't see this in the first place.




Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:2765-2768
+  // Debatable that there is no warning. Currently we don't track in the scoped
+  // object whether it is active, but just check if the contained locks can be
+  // reacquired. Here they can, because mu has been unlocked manually.
+  scope.Lock();

I have a local patch that addresses this, but I think it should be discussed 
separately as it introduces additional changes.


Repository:
  rC Clang

https://reviews.llvm.org/D49885



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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

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

Proper capitalization of member function, reduction of test code.


Repository:
  rC Clang

https://reviews.llvm.org/D49885

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
@@ -2621,6 +2621,171 @@
 } // end namespace ReleasableScopedLock
 
 
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+  RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+struct SharedTraits {};
+struct ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+  RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+  RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void ReaderUnlock() UNLOCK_FUNCTION();
+
+  void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void relock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  x = 2;
+  scope.Unlock();
+
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  x = 4;
+}
+
+void relockExclusive() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.ReaderUnlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.Lock();
+  print(x);
+  x = 4;
+
+  scope.DemoteExclusive();
+  print(x);
+  x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{});
+  print(x);
+  x = 2;
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.ReaderLock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.PromoteShared();
+  print(x);
+  x = 5;
+}
+
+void doubleUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  mu.Unlock();
+  // Debatable that there is no warning. Currently we don't track in the scoped
+  // object whether it is active, but just check if the contained locks can be
+  // reacquired. Here they can, because mu has been unlocked manually.
+  scope.Lock();
+}
+
+void directRelock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  mu.Lock();
+  // Similarly debatable that there is no warning.
+  scope.Unlock();
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.~RelockableExclusiveMutexLock();
+  scope.Lock(); // Should be UB, so we don't really care.
+}
+
+class SCOPED_LOCKABLE MemberLock {
+ public:
+  MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  ~MemberLock() UNLOCK_FUNCTION(mutex);
+  void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  Mutex mutex;
+};
+
+void relockShared2() {
+  MemberLock lock;
+  lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
+}
+
+class SCOPED_LOCKABLE WeirdScope {
+private:
+  Mutex *other;
+
+public:
+  WeirdScope(Mutex *mutex) EXCLUSIVE_LOCK_FUNCTION(mutex);
+  void unlock() EXCLUSIVE_UNLOCK_FUNCTION() EXCLUSIVE_UNLOCK_FUNCTION(other);
+  void lock() EXCLUSIVE_LOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION(other);
+  ~WeirdScope() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void requireOther() EXCLUSIVE_LOCKS_REQUIRED(other);
+};
+
+void relockWeird()
+{
+  WeirdScope scope(&mu);
+  x = 1;
+  scope.unlock(); // expected-warning {{releasing mutex 'scope.other' that was not held}}
+  x = 2; // \
+// expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.requireOther(); // \
+// expected-warni

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

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

Formatting changes.


Repository:
  rC Clang

https://reviews.llvm.org/D49885

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
@@ -2621,6 +2621,171 @@
 } // end namespace ReleasableScopedLock
 
 
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+  RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+struct SharedTraits {};
+struct ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+  RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+  RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void ReaderUnlock() UNLOCK_FUNCTION();
+
+  void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void relock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  x = 2;
+  scope.Unlock();
+
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  x = 4;
+}
+
+void relockExclusive() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.ReaderUnlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.Lock();
+  print(x);
+  x = 4;
+
+  scope.DemoteExclusive();
+  print(x);
+  x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{});
+  print(x);
+  x = 2;
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.ReaderLock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.PromoteShared();
+  print(x);
+  x = 5;
+}
+
+void doubleUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  mu.Unlock();
+  // Debatable that there is no warning. Currently we don't track in the scoped
+  // object whether it is active, but just check if the contained locks can be
+  // reacquired. Here they can, because mu has been unlocked manually.
+  scope.Lock();
+}
+
+void directRelock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  mu.Lock();
+  // Similarly debatable that there is no warning.
+  scope.Unlock();
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.~RelockableExclusiveMutexLock();
+  scope.Lock(); // Should be UB, so we don't really care.
+}
+
+class SCOPED_LOCKABLE MemberLock {
+ public:
+  MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  ~MemberLock() UNLOCK_FUNCTION(mutex);
+  void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  Mutex mutex;
+};
+
+void relockShared2() {
+  MemberLock lock;
+  lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
+}
+
+class SCOPED_LOCKABLE WeirdScope {
+private:
+  Mutex *other;
+
+public:
+  WeirdScope(Mutex *mutex) EXCLUSIVE_LOCK_FUNCTION(mutex);
+  void unlock() EXCLUSIVE_UNLOCK_FUNCTION() EXCLUSIVE_UNLOCK_FUNCTION(other);
+  void lock() EXCLUSIVE_LOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION(other);
+  ~WeirdScope() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void requireOther() EXCLUSIVE_LOCKS_REQUIRED(other);
+};
+
+void relockWeird()
+{
+  WeirdScope scope(&mu);
+  x = 1;
+  scope.unlock(); // expected-warning {{releasing mutex 'scope.other' that was not held}}
+  x = 2; // \
+// expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.requireOther(); // \
+// expected-warning {{calling function 'requireOther' requires 

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

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

Reformat tests. I promise, this is the last one.


Repository:
  rC Clang

https://reviews.llvm.org/D49885

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
@@ -2621,6 +2621,170 @@
 } // end namespace ReleasableScopedLock
 
 
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+  RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+struct SharedTraits {};
+struct ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+  RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+  RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void ReaderUnlock() UNLOCK_FUNCTION();
+
+  void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void relock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  x = 2;
+  scope.Unlock();
+
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  x = 4;
+}
+
+void relockExclusive() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.ReaderUnlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.Lock();
+  print(x);
+  x = 4;
+
+  scope.DemoteExclusive();
+  print(x);
+  x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{});
+  print(x);
+  x = 2;
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.ReaderLock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.PromoteShared();
+  print(x);
+  x = 5;
+}
+
+void doubleUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  mu.Unlock();
+  // Debatable that there is no warning. Currently we don't track in the scoped
+  // object whether it is active, but just check if the contained locks can be
+  // reacquired. Here they can, because mu has been unlocked manually.
+  scope.Lock();
+}
+
+void directRelock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  mu.Lock();
+  // Similarly debatable that there is no warning.
+  scope.Unlock();
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.~RelockableExclusiveMutexLock();
+  scope.Lock(); // Should be UB, so we don't really care.
+}
+
+class SCOPED_LOCKABLE MemberLock {
+public:
+  MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  ~MemberLock() UNLOCK_FUNCTION(mutex);
+  void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  Mutex mutex;
+};
+
+void relockShared2() {
+  MemberLock lock;
+  lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
+}
+
+class SCOPED_LOCKABLE WeirdScope {
+private:
+  Mutex *other;
+
+public:
+  WeirdScope(Mutex *mutex) EXCLUSIVE_LOCK_FUNCTION(mutex);
+  void unlock() EXCLUSIVE_UNLOCK_FUNCTION() EXCLUSIVE_UNLOCK_FUNCTION(other);
+  void lock() EXCLUSIVE_LOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION(other);
+  ~WeirdScope() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void requireOther() EXCLUSIVE_LOCKS_REQUIRED(other);
+};
+
+void relockWeird() {
+  WeirdScope scope(&mu);
+  x = 1;
+  scope.unlock(); // expected-warning {{releasing mutex 'scope.other' that was not held}}
+  x = 2; // \
+// expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.requireOther(); // \
+// expected-warning {{calling functi

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340459: Thread safety analysis: Allow relockable scopes 
(authored by aaronpuchert, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49885

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

Index: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -2621,6 +2621,170 @@
 } // end namespace ReleasableScopedLock
 
 
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+  RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+struct SharedTraits {};
+struct ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+  RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+  RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void ReaderUnlock() UNLOCK_FUNCTION();
+
+  void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void relock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  x = 2;
+  scope.Unlock();
+
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  x = 4;
+}
+
+void relockExclusive() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.ReaderUnlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.Lock();
+  print(x);
+  x = 4;
+
+  scope.DemoteExclusive();
+  print(x);
+  x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{});
+  print(x);
+  x = 2;
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.ReaderLock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.PromoteShared();
+  print(x);
+  x = 5;
+}
+
+void doubleUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void directUnlock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  mu.Unlock();
+  // Debatable that there is no warning. Currently we don't track in the scoped
+  // object whether it is active, but just check if the contained locks can be
+  // reacquired. Here they can, because mu has been unlocked manually.
+  scope.Lock();
+}
+
+void directRelock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  mu.Lock();
+  // Similarly debatable that there is no warning.
+  scope.Unlock();
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.~RelockableExclusiveMutexLock();
+  scope.Lock(); // Should be UB, so we don't really care.
+}
+
+class SCOPED_LOCKABLE MemberLock {
+public:
+  MemberLock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  ~MemberLock() UNLOCK_FUNCTION(mutex);
+  void Lock() EXCLUSIVE_LOCK_FUNCTION(mutex);
+  Mutex mutex;
+};
+
+void relockShared2() {
+  MemberLock lock;
+  lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
+}
+
+class SCOPED_LOCKABLE WeirdScope {
+private:
+  Mutex *other;
+
+public:
+  WeirdScope(Mutex *mutex) EXCLUSIVE_LOCK_FUNCTION(mutex);
+  void unlock() EXCLUSIVE_UNLOCK_FUNCTION() EXCLUSIVE_UNLOCK_FUNCTION(other);
+  void lock() EXCLUSIVE_LOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION(other);
+  ~WeirdScope() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void requireOther() EXCLUSIVE_LOCKS_REQUIRED(other);
+};
+
+void relockWeird() {
+  WeirdScope scope(&mu);
+  x = 1;
+  scope.unlock(); // expected-warning {{releasing mutex 'scope.other' that was not held}}
+  x = 2; // \
+// ex

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: lib/Analysis/ThreadSafety.cpp:932
+  // We're relocking the underlying mutexes. Warn on double locking.
+  if (FSet.findLock(FactMan, UnderCp))
+Handler.handleDoubleLock(DiagKind, UnderCp.toString(), entry.loc());

delesley wrote:
> Minor nit.  Use curly braces on the if, to match the else.
Removing the braces [was 
suggested](https://reviews.llvm.org/D49885?id=157599#inline-439256) by 
@aaron.ballman in an earlier patch set, but I can just add them in again. I 
slightly favor having them, but I don't feel strongly either way.


Repository:
  rL LLVM

https://reviews.llvm.org/D49885



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

[PATCH] D51187: Thread safety analysis: Warn on double (un-)lock of scoped capability

[Aaron Puchert via Phabricator via cfe-commits]

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

We now warn when acquiring or releasing a scoped capability a second
time, not just if the underlying mutexes have been acquired or released
a second time. It's debatable whether that should really be a warning,
but there seem to be more advantages.


Repository:
  rC Clang

https://reviews.llvm.org/D51187

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
@@ -2605,16 +2605,16 @@
 void Foo::test4() {
   ReleasableMutexLock rlock(&mu_);
   rlock.Release();
-  rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not held}}
+  rlock.Release();  // expected-warning {{releasing mutex 'rlock' that was not held}}
 }
 
 void Foo::test5() {
   ReleasableMutexLock rlock(&mu_);
   if (c) {
 rlock.Release();
   }
   // no warning on join point for managed lock.
-  rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not held}}
+  rlock.Release();  // expected-warning {{releasing mutex 'rlock' that was not held}}
 }
 
 
@@ -2704,36 +2704,32 @@
 void doubleUnlock() {
   RelockableExclusiveMutexLock scope(&mu);
   scope.Unlock();
-  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+  scope.Unlock(); // expected-warning {{releasing mutex 'scope' that was not held}}
 }
 
 void doubleLock1() {
   RelockableExclusiveMutexLock scope(&mu);
-  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+  scope.Lock(); // expected-warning {{acquiring mutex 'scope' that is already held}}
 }
 
 void doubleLock2() {
   RelockableExclusiveMutexLock scope(&mu);
   scope.Unlock();
   scope.Lock();
-  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+  scope.Lock(); // expected-warning {{acquiring mutex 'scope' that is already held}}
 }
 
 void directUnlock() {
   RelockableExclusiveMutexLock scope(&mu);
   mu.Unlock();
-  // Debatable that there is no warning. Currently we don't track in the scoped
-  // object whether it is active, but just check if the contained locks can be
-  // reacquired. Here they can, because mu has been unlocked manually.
-  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'scope' that is already held}}
 }
 
 void directRelock() {
   RelockableExclusiveMutexLock scope(&mu);
   scope.Unlock();
   mu.Lock();
-  // Similarly debatable that there is no warning.
-  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'scope' that was not held}}
 }
 
 // Doesn't make a lot of sense, just making sure there is no crash.
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -144,11 +144,11 @@
 ThreadSafetyHandler &Handler) const = 0;
   virtual void handleLock(FactSet &FSet, FactManager &FactMan,
   const FactEntry &entry, ThreadSafetyHandler &Handler,
-  StringRef DiagKind) const = 0;
+  StringRef DiagKind) = 0;
   virtual void handleUnlock(FactSet &FSet, FactManager &FactMan,
 const CapabilityExpr &Cp, SourceLocation UnlockLoc,
 bool FullyRemove, ThreadSafetyHandler &Handler,
-StringRef DiagKind) const = 0;
+StringRef DiagKind) = 0;
 
   // Return true if LKind >= LK, where exclusive > shared
   bool isAtLeast(LockKind LK) {
@@ -877,15 +877,14 @@
   }
 
   void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
-  ThreadSafetyHandler &Handler,
-  StringRef DiagKind) const override {
+  ThreadSafetyHandler &Handler, StringRef DiagKind) override {
 Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc());
   }
 
   void handleUnlock(FactSet &FSet, FactManager &FactMan,
 const CapabilityExpr &Cp, SourceLocation UnlockLoc,
 bool FullyRemove, ThreadSafetyHandler &Handler,
-StringRef DiagKind) const override {
+StringRef DiagKind) override {
 FSet.removeLock(FactMan, Cp);
 if (!Cp.negative()) {
   FSet.addLock(FactMan, llvm::make_unique(
@@ -897,6 +896,7 @@
 class ScopedLockableFactEntry : public FactEntry {
 private:
   SmallVector UnderlyingMutexes;
+  bool Locked = true; // Are the UnderlyingMutexes currently locked?
 
 public:
   ScopedLockableFactEntry(const CapabilityExpr &CE, SourceLocation Loc,
@@ -923,8 +923,11 @@
   }
 
   void handleLock(FactSet &FSet, FactManager &FactM

[PATCH] D51187: Thread safety analysis: Warn on double (un-)lock of scoped capability

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Sure. As I wrote in the commit message, I'm not sure about it myself, but I 
think it's worth a discussion. Maybe I should have tagged it as RFC.

Releasable scopes need a way of knowing whether the lock is currently held to 
prevent double unlocking in the destructor. There are basically two ways to 
achieve that: one can ask the underlying mutex if it is locked, or keep track 
of the lock status. The second might be the only option available, as not all 
mutex implementations offer a way to find out if they are currently held.

Suppose we have something like:

  class ReleasableScope {
Mutex *mu;
bool Locked;
  
  public:
ReleasableScope(Mutex *mu) : mu(mu), Locked(true) {
  mu->Lock();
}
void Unlock() { mu->Unlock(); Locked = false; }
~ReleasableScope() { if (Locked) mu->Unlock(); }
  };

Then it can be a problem if someone works directly on the underlying mutex 
without informing the scoped capability:

  Mutex mu;
  
  void test1() {
ReleasableScope scope(&mu);
mu.Unlock();
  } // ~ReleasableScope unlocks mu again, but we don't warn about double unlock.
  
  void test2() {
ReleasableScope scope(&mu);
scope.Unlock()
mu.Lock();
  } // ~ReleasableScope does not unlock, but we don't warn about the lock still 
being held.

In the `LockableFactEntry` we call `mu` the "managed" mutex of `scope` and I 
think that implies that for its lifetime only `scope` should touch `mu`. What 
we do in `test1` is akin to manually `delete`ing the underlying pointer of a 
`std::unique_ptr`. I would actually consider releasing `mu` again in 
`ScopedLockableFactEntry::handleUnlock` in the epilogue of `test1` and not 
releasing it in the epilogue of `test2`, because we think it is still 
locked/already unlocked. Then we would emit warnings in both cases.

On the other hand, if the scoped capability can (and does) ask the underlying 
mutex for its state before doing anything, we might be completely fine, as you 
stated.

So I'm really not sure, but I think there are some arguments that can be made 
for introducing this warning (and possibly more).

We could also consider making this part of the `-Wthread-safety-beta` warnings, 
so that we don't break anyone's `-Werror=thread-safety` builds.


Repository:
  rC Clang

https://reviews.llvm.org/D51187



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

[PATCH] D51187: Thread safety analysis: Warn on double (un-)lock of scoped capability

[Aaron Puchert via Phabricator via cfe-commits]

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

Use Locked flag to determine whether to unlock on destruction

Instead of unlocking the mutexes that are still available while not complaining 
about those that aren't, we use the status of the scoped capability to 
determine whether to unlock the underlying mutexes.

This way we will attempt to unlock the mutex even if the user unlocked it 
manually, warning if it is no longer available, and we will not attempt to 
unlock if we are already released, hence warning if it is manually acquired 
again.


Repository:
  rC Clang

https://reviews.llvm.org/D51187

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
@@ -1729,7 +1729,7 @@
 MutexLock mulock_a(&mu1);
 MutexLock mulock_b(&mu1); // \
   // expected-warning {{acquiring mutex 'mu1' that is already held}}
-  }
+  } // expected-warning {{releasing mutex 'mu1' that was not held}}
 
   void foo4() {
 MutexLock mulock1(&mu1), mulock2(&mu2);
@@ -2580,6 +2580,8 @@
   void test3();
   void test4();
   void test5();
+  void test6();
+  void test7();
 };
 
 
@@ -2605,18 +2607,29 @@
 void Foo::test4() {
   ReleasableMutexLock rlock(&mu_);
   rlock.Release();
-  rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not held}}
+  rlock.Release();  // expected-warning {{releasing mutex 'rlock' that was not held}}
 }
 
 void Foo::test5() {
   ReleasableMutexLock rlock(&mu_);
   if (c) {
 rlock.Release();
   }
   // no warning on join point for managed lock.
-  rlock.Release();  // expected-warning {{releasing mutex 'mu_' that was not held}}
+  rlock.Release();  // expected-warning {{releasing mutex 'rlock' that was not held}}
 }
 
+void Foo::test6() {
+  ReleasableMutexLock rlock(&mu_);
+  mu_.Unlock();
+} // expected-warning {{releasing mutex 'mu_' that was not held}}
+
+void Foo::test7() {
+  ReleasableMutexLock rlock(&mu_);
+  rlock.Release();
+  mu_.Lock(); // expected-note {{mutex acquired here}}
+} // expected-warning {{mutex 'mu_' is still held at the end of function}}
+
 
 } // end namespace ReleasableScopedLock
 
@@ -2704,37 +2717,33 @@
 void doubleUnlock() {
   RelockableExclusiveMutexLock scope(&mu);
   scope.Unlock();
-  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+  scope.Unlock(); // expected-warning {{releasing mutex 'scope' that was not held}}
 }
 
 void doubleLock1() {
   RelockableExclusiveMutexLock scope(&mu);
-  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+  scope.Lock(); // expected-warning {{acquiring mutex 'scope' that is already held}}
 }
 
 void doubleLock2() {
   RelockableExclusiveMutexLock scope(&mu);
   scope.Unlock();
   scope.Lock();
-  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+  scope.Lock(); // expected-warning {{acquiring mutex 'scope' that is already held}}
 }
 
 void directUnlock() {
   RelockableExclusiveMutexLock scope(&mu);
   mu.Unlock();
-  // Debatable that there is no warning. Currently we don't track in the scoped
-  // object whether it is active, but just check if the contained locks can be
-  // reacquired. Here they can, because mu has been unlocked manually.
-  scope.Lock();
-}
+  scope.Lock(); // expected-warning {{acquiring mutex 'scope' that is already held}}
+} // expected-warning {{releasing mutex 'mu' that was not held}}
 
 void directRelock() {
   RelockableExclusiveMutexLock scope(&mu);
   scope.Unlock();
-  mu.Lock();
-  // Similarly debatable that there is no warning.
-  scope.Unlock();
-}
+  mu.Lock(); // expected-note {{mutex acquired here}}
+  scope.Unlock(); // expected-warning {{releasing mutex 'scope' that was not held}}
+} // expected-warning {{mutex 'mu' is still held at the end of function}}
 
 // Doesn't make a lot of sense, just making sure there is no crash.
 void destructLock() {
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -144,11 +144,11 @@
 ThreadSafetyHandler &Handler) const = 0;
   virtual void handleLock(FactSet &FSet, FactManager &FactMan,
   const FactEntry &entry, ThreadSafetyHandler &Handler,
-  StringRef DiagKind) const = 0;
+  StringRef DiagKind) = 0;
   virtual void handleUnlock(FactSet &FSet, FactManager &FactMan,
 const CapabilityExpr &Cp, SourceLocation UnlockLoc,
 bool FullyRemove, ThreadSafetyHandler &Handler,
-StringRef DiagKind) const = 0;
+StringRef DiagKind) = 0;
 

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

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

When thread safety annotations are used without capability arguments,
they are assumed to apply to `this` instead. So we warn when either
`this` doesn't exist, or the class is not a capability type.

This is based on earlier work by Josh Gao that was committed in r310403,
but reverted in r310698 because it didn't properly work in template
classes. See also https://reviews.llvm.org/D36237.

The solution is not to go via the QualType of `this`, which is then a
template type, hence the attributes are not known because it could be
specialized. Instead we look directly at the class in which we are
contained.

Additionally I grouped two of the warnings together. There are two
issues here: the existence of `this`, which requires us to be a
non-static member function, and the appropriate annotation on the class
we are contained in. So we don't distinguish between not being in a
class and being static, because in both cases we don't have `this`.

Fixes PR38399.


Repository:
  rC Clang

https://reviews.llvm.org/D51901

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-capabilities.c
  test/SemaCXX/warn-thread-safety-parsing.cpp

Index: test/SemaCXX/warn-thread-safety-parsing.cpp
===
--- test/SemaCXX/warn-thread-safety-parsing.cpp
+++ test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -572,11 +572,11 @@
 
 // takes zero or more arguments, all locks (vars/fields)
 
-void elf_function() EXCLUSIVE_LOCK_FUNCTION();
+void elf_function() EXCLUSIVE_LOCK_FUNCTION(); // expected-warning {{'exclusive_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 void elf_function_args() EXCLUSIVE_LOCK_FUNCTION(mu1, mu2);
 
-int elf_testfn(int y) EXCLUSIVE_LOCK_FUNCTION();
+int elf_testfn(int y) EXCLUSIVE_LOCK_FUNCTION(); // expected-warning {{'exclusive_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int elf_testfn(int y) {
   int x EXCLUSIVE_LOCK_FUNCTION() = y; // \
@@ -591,7 +591,8 @@
  private:
   int test_field EXCLUSIVE_LOCK_FUNCTION(); // \
 // expected-warning {{'exclusive_lock_function' attribute only applies to functions}}
-  void test_method() EXCLUSIVE_LOCK_FUNCTION();
+  void test_method() EXCLUSIVE_LOCK_FUNCTION(); // \
+// expected-warning {{'exclusive_lock_function' attribute without capability arguments can only be applied in a class annotated with 'capability' or 'scoped_lockable' attribute, but 'ElfFoo' isn't}}
 };
 
 class EXCLUSIVE_LOCK_FUNCTION() ElfTestClass { // \
@@ -644,11 +645,11 @@
 
 // takes zero or more arguments, all locks (vars/fields)
 
-void slf_function() SHARED_LOCK_FUNCTION();
+void slf_function() SHARED_LOCK_FUNCTION(); // expected-warning {{'shared_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 void slf_function_args() SHARED_LOCK_FUNCTION(mu1, mu2);
 
-int slf_testfn(int y) SHARED_LOCK_FUNCTION();
+int slf_testfn(int y) SHARED_LOCK_FUNCTION(); // expected-warning {{'shared_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int slf_testfn(int y) {
   int x SHARED_LOCK_FUNCTION() = y; // \
@@ -666,7 +667,8 @@
  private:
   int test_field SHARED_LOCK_FUNCTION(); // \
 // expected-warning {{'shared_lock_function' attribute only applies to functions}}
-  void test_method() SHARED_LOCK_FUNCTION();
+  void test_method() SHARED_LOCK_FUNCTION(); // \
+// expected-warning {{'shared_lock_function' attribute without capability arguments can only be applied in a class annotated with 'capability' or 'scoped_lockable' attribute, but 'SlfFoo' isn't}}
 };
 
 class SHARED_LOCK_FUNCTION() SlfTestClass { // \
@@ -717,14 +719,16 @@
 // takes a mandatory boolean or integer argument specifying the retval
 // plus an optional list of locks (vars/fields)
 
-void etf_function() __attribute__((exclusive_trylock_function));  // \
+void etf_function() __attribute__((exclusive_trylock_function)); // \
   // expected-error {{'exclusive_trylock_function' attribute takes at least 1 argument}}
 
 void etf_function_args() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2);
 
-void etf_function_arg() EXCLUSIVE_TRYLOCK_FUNCTION(1);
+void etf_function_arg() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+  // expected-warning {{'exclusive_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
-int etf_testfn(int y) EXCLUSIVE_TRYLOCK_FUNCTION(1);
+int etf_testfn(int y) EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+  // expected-warning {{'exclusive_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int etf_testfn(int y) {
   int x EXCLUSIVE_TRYLOC

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

The second warning message is a bit long, so if any of you have a better idea 
I'd welcome it.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3016-3017
+def warn_thread_attribute_not_on_capability_member : Warning<
+  "%0 attribute without capability arguments can only be applied in a class "
+  "annotated with 'capability' or 'scoped_lockable' attribute, but %1 isn't">,
+  InGroup, DefaultIgnore;

Alternative wording: "%0 attribute without capability arguments refers to 
'this', but %1 isn't annotated with 'capability' or 'scoped_lockable' 
attribute".


Repository:
  rC Clang

https://reviews.llvm.org/D51901



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

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

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

Improved handling of type-dependent base classes and slightly reworded warning 
message.


Repository:
  rC Clang

https://reviews.llvm.org/D51901

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-capabilities.c
  test/SemaCXX/warn-thread-safety-parsing.cpp

Index: test/SemaCXX/warn-thread-safety-parsing.cpp
===
--- test/SemaCXX/warn-thread-safety-parsing.cpp
+++ test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -572,11 +572,11 @@
 
 // takes zero or more arguments, all locks (vars/fields)
 
-void elf_function() EXCLUSIVE_LOCK_FUNCTION();
+void elf_function() EXCLUSIVE_LOCK_FUNCTION(); // expected-warning {{'exclusive_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 void elf_function_args() EXCLUSIVE_LOCK_FUNCTION(mu1, mu2);
 
-int elf_testfn(int y) EXCLUSIVE_LOCK_FUNCTION();
+int elf_testfn(int y) EXCLUSIVE_LOCK_FUNCTION(); // expected-warning {{'exclusive_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int elf_testfn(int y) {
   int x EXCLUSIVE_LOCK_FUNCTION() = y; // \
@@ -591,7 +591,8 @@
  private:
   int test_field EXCLUSIVE_LOCK_FUNCTION(); // \
 // expected-warning {{'exclusive_lock_function' attribute only applies to functions}}
-  void test_method() EXCLUSIVE_LOCK_FUNCTION();
+  void test_method() EXCLUSIVE_LOCK_FUNCTION(); // \
+// expected-warning {{'exclusive_lock_function' attribute without capability arguments refers to 'this', but 'ElfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
 };
 
 class EXCLUSIVE_LOCK_FUNCTION() ElfTestClass { // \
@@ -644,11 +645,11 @@
 
 // takes zero or more arguments, all locks (vars/fields)
 
-void slf_function() SHARED_LOCK_FUNCTION();
+void slf_function() SHARED_LOCK_FUNCTION(); // expected-warning {{'shared_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 void slf_function_args() SHARED_LOCK_FUNCTION(mu1, mu2);
 
-int slf_testfn(int y) SHARED_LOCK_FUNCTION();
+int slf_testfn(int y) SHARED_LOCK_FUNCTION(); // expected-warning {{'shared_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int slf_testfn(int y) {
   int x SHARED_LOCK_FUNCTION() = y; // \
@@ -666,7 +667,8 @@
  private:
   int test_field SHARED_LOCK_FUNCTION(); // \
 // expected-warning {{'shared_lock_function' attribute only applies to functions}}
-  void test_method() SHARED_LOCK_FUNCTION();
+  void test_method() SHARED_LOCK_FUNCTION(); // \
+// expected-warning {{'shared_lock_function' attribute without capability arguments refers to 'this', but 'SlfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
 };
 
 class SHARED_LOCK_FUNCTION() SlfTestClass { // \
@@ -717,14 +719,16 @@
 // takes a mandatory boolean or integer argument specifying the retval
 // plus an optional list of locks (vars/fields)
 
-void etf_function() __attribute__((exclusive_trylock_function));  // \
+void etf_function() __attribute__((exclusive_trylock_function)); // \
   // expected-error {{'exclusive_trylock_function' attribute takes at least 1 argument}}
 
 void etf_function_args() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2);
 
-void etf_function_arg() EXCLUSIVE_TRYLOCK_FUNCTION(1);
+void etf_function_arg() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+  // expected-warning {{'exclusive_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
-int etf_testfn(int y) EXCLUSIVE_TRYLOCK_FUNCTION(1);
+int etf_testfn(int y) EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+  // expected-warning {{'exclusive_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int etf_testfn(int y) {
   int x EXCLUSIVE_TRYLOCK_FUNCTION(1) = y; // \
@@ -739,7 +743,8 @@
  private:
   int test_field EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
 // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}}
-  void test_method() EXCLUSIVE_TRYLOCK_FUNCTION(1);
+  void test_method() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+// expected-warning {{'exclusive_trylock_function' attribute without capability arguments refers to 'this', but 'EtfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
 };
 
 class EXCLUSIVE_TRYLOCK_FUNCTION(1) EtfTestClass { // \
@@ -760,7 +765,8 @@
 int etf_function_6() EXCLUSIVE_TRYLOCK_FUNCTION(1, muRef);
 int etf_function_7() EXCLUSIVE_TRYLOCK_FUNCTION(1, muDoubleWrapper.getWrapper()->getMu());
 int etf_functetfn_8() EXCLUSIVE_TRYLOCK_FUNCTION(1, muPointer);
-int etf_function_9() EXCLUSIVE_TRYLOCK_FUNCTION(true);
+int etf_function_9() EXCLUSIVE_TRYLOCK_FUNCTION(true); // \
+  // expected-warning {{'exclusive_try

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked an inline comment as done.
aaronpuchert added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3016-3017
+def warn_thread_attribute_not_on_capability_member : Warning<
+  "%0 attribute without capability arguments can only be applied in a class "
+  "annotated with 'capability' or 'scoped_lockable' attribute, but %1 isn't">,
+  InGroup, DefaultIgnore;

jmgao wrote:
> aaronpuchert wrote:
> > Alternative wording: "%0 attribute without capability arguments refers to 
> > 'this', but %1 isn't annotated with 'capability' or 'scoped_lockable' 
> > attribute".
> Maybe something like "implicit 'this' argument for %0 attribute isn't 
> annotated with 'capability' or 'scoped_lockable" attribute"?
I like that it's short, but technically we want the argument's type—not the 
argument itself—to be annotated and I worry that this might not be clear. In 
the following warning message we talk about the "argument whose type is 
annotated" for example.



Comment at: test/SemaCXX/warn-thread-safety-parsing.cpp:1286-1287
+
+// FIXME: warn on template instantiation.
+template struct SLTemplateDerived;
+

The explicit instantiation appears in the AST in its full glory, but I'm not 
sure how make the analysis run over it. I saw that 
`Sema::ProcessDeclAttributeList` is called from 
`Sema::ActOnExplicitInstantiation`, but that seems to check the attributes on 
the class/function itself, not on members.


Repository:
  rC Clang

https://reviews.llvm.org/D51901



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

[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability

[Aaron Puchert via Phabricator via cfe-commits]

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

This doesn't work with loops yet:

  void relockLoop() {
RelockableExclusiveMutexLock scope(&mu);
  
while (b) {
  scope.Unlock(); // releasing mutex 'scope' that was not held
  scope.Lock();   // acquiring mutex 'mu' that is already held
}
  }

There should be no warnings here — this code is fine.


Repository:
  rC Clang

https://reviews.llvm.org/D51187



___
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 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] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

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

It's already allowed to prematurely release a scoped lock, now we also
allow relocking it again, possibly even in another mode.

Arguably the solution is not very elegant, but maybe that can only be
solved by comprehensive refactoring.


Repository:
  rC Clang

https://reviews.llvm.org/D49885

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
@@ -2621,6 +2621,154 @@
 } // end namespace ReleasableScopedLock
 
 
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+  RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE RelockableSharedMutexLock {
+public:
+  RelockableSharedMutexLock(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+  ~RelockableSharedMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() SHARED_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+class SharedTraits {};
+class ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+  RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+  RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void ReaderUnlock() UNLOCK_FUNCTION();
+
+  void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void write() {
+  RelockableExclusiveMutexLock scope(&mu);
+  x = 2;
+  scope.Unlock();
+
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  x = 4;
+}
+
+void read() {
+  RelockableSharedMutexLock scope(&mu);
+  print(x);
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockExclusive() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.ReaderUnlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.Lock();
+  print(x);
+  x = 4;
+
+  scope.DemoteExclusive();
+  print(x);
+  x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{});
+  print(x);
+  x = 2;
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.ReaderLock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.PromoteShared();
+  print(x);
+  x = 5;
+}
+
+void doubleUnlock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleUnlock2() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock3() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock4() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.~RelockableExclusiveMutexLock();
+  scope.Lock(); // Maybe this should warn.
+} // expected-warning {{releasing mutex 'scope' that was not held}}
+
+} // end namespace RelockableScopedLock
+
+
 namespace TrylockFunctionTest {
 
 class Foo {
Index: lib/Analysis/Thread

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Imagine having a producer loop, where we check a loop condition while holding a 
mutex, but release it in the loop body to let other producers/consumers do 
their work. In that scenario it makes sense to allow "relocking" a scope.

  RelockableScope Scope(mu);
  while (WorkToDo()) {
  Scope.Unlock();
  // Produce something...
  Scope.Lock();
  PublishWork();
  }




Comment at: lib/Analysis/ThreadSafety.cpp:960-961
+FSet.removeLock(FactMan, !UnderCp);
+FSet.addLock(FactMan, llvm::make_unique(UnderCp, LK,
+   RelockLoc));
+  }

Looks a bit weird, but `clang-format` told me to do that.



Comment at: lib/Analysis/ThreadSafety.cpp:1318-1321
+// FIXME: It's possible to manually destruct the scope and then relock it.
+// Should that be a separate warning? For now we pretend nothing happened.
+// It's undefined behavior, so not related to thread safety.
+return;

Not sure about this part, but it's probably not worth worrying about. A user 
would have to call a member function on a scoped object after manually ending 
its lifetime by calling the destructor, see test 
`RelockableScopedLock::destructLock`.



Comment at: lib/Analysis/ThreadSafety.cpp:1324-1325
+
+  // We should only land here if Cp is a scoped capability, so if we have any
+  // fact, it must be a ScopedLockableFactEntry.
+  auto SLDat = static_cast(LDat);

I hope this reasoning is sufficient to justify the following static_cast, 
otherwise I need to introduce LLVM-style RTTI into `FactEntry`.


Repository:
  rC Clang

https://reviews.llvm.org/D49885



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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

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

Formatting changes suggested by Aaron Ballman.


Repository:
  rC Clang

https://reviews.llvm.org/D49885

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
@@ -2621,6 +2621,154 @@
 } // end namespace ReleasableScopedLock
 
 
+namespace RelockableScopedLock {
+
+class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
+public:
+  RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableExclusiveMutexLock() EXCLUSIVE_UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE RelockableSharedMutexLock {
+public:
+  RelockableSharedMutexLock(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+  ~RelockableSharedMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() SHARED_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+};
+
+class SharedTraits {};
+class ExclusiveTraits {};
+
+class SCOPED_LOCKABLE RelockableMutexLock {
+public:
+  RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
+  RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
+  ~RelockableMutexLock() UNLOCK_FUNCTION();
+
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void ReaderUnlock() UNLOCK_FUNCTION();
+
+  void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
+  void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
+};
+
+Mutex mu;
+int x GUARDED_BY(mu);
+
+void print(int);
+
+void write() {
+  RelockableExclusiveMutexLock scope(&mu);
+  x = 2;
+  scope.Unlock();
+
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  x = 4;
+}
+
+void read() {
+  RelockableSharedMutexLock scope(&mu);
+  print(x);
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  x = 3; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.Lock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockExclusive() {
+  RelockableMutexLock scope(&mu, SharedTraits{});
+  print(x);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  scope.ReaderUnlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.Lock();
+  print(x);
+  x = 4;
+
+  scope.DemoteExclusive();
+  print(x);
+  x = 5; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void relockShared() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{});
+  print(x);
+  x = 2;
+  scope.Unlock();
+
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+
+  scope.ReaderLock();
+  print(x);
+  x = 4; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+
+  scope.PromoteShared();
+  print(x);
+  x = 5;
+}
+
+void doubleUnlock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleUnlock2() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+}
+
+void doubleLock1() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock2() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock3() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleLock4() {
+  RelockableSharedMutexLock scope(&mu);
+  scope.Unlock();
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+// Doesn't make a lot of sense, just making sure there is no crash.
+void destructLock() {
+  RelockableExclusiveMutexLock scope(&mu);
+  scope.~RelockableExclusiveMutexLock();
+  scope.Lock(); // Maybe this should warn.
+} // expected-warning {{releasing mutex 'scope' that was not held}}
+
+} // end namespace RelockableScopedLock
+
+
 namespace TrylockFunctionTest {
 
 class Foo {
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -86,8 +86,8 @@
 
 namespace {
 
-/// A set of CapabilityInfo objects, which are compiled 

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked 3 inline comments as done.
aaronpuchert added inline comments.



Comment at: lib/Analysis/ThreadSafety.cpp:960-961
+FSet.removeLock(FactMan, !UnderCp);
+FSet.addLock(FactMan, llvm::make_unique(UnderCp, LK,
+   RelockLoc));
+  }

aaron.ballman wrote:
> aaronpuchert wrote:
> > Looks a bit weird, but `clang-format` told me to do that.
> This line looks correct to me, but the `else` statement above doesn't match 
> our usual formatting rules, so I'm worried you're running clang-format in a 
> way that's not picking up the LLVM coding style options.
You're right about the `else`, not sure what happened there.


Repository:
  rC Clang

https://reviews.llvm.org/D49885



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

[PATCH] D50110: Handle shared release attributes correctly

[Aaron Puchert via Phabricator via cfe-commits]

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

Repository:
  rC Clang

https://reviews.llvm.org/D50110

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
@@ -4078,6 +4078,14 @@
 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();
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -2242,8 +2242,8 @@
 // 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(Attr)) {


Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4078,6 +4078,14 @@
 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();
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -2242,8 +2242,8 @@
 // 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(Attr)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50110: Handle shared release attributes correctly

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:4081-4085
+  void unlockExclusive() EXCLUSIVE_UNLOCK_FUNCTION(mu_) {
+mu_.Unlock();
+  }
+
+  void unlockShared() SHARED_UNLOCK_FUNCTION(mu_) {

aaron.ballman wrote:
> Nothing calls either of these functions within the test; is that intended?
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.


Repository:
  rC Clang

https://reviews.llvm.org/D50110



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

[PATCH] D50110: Handle shared release attributes correctly

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D50110



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

[PATCH] D50110: Handle shared release attributes correctly

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D50110



___
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

[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] 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(&mu);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void simpleShared() SHARED_LOCKS_REQUIRED(mu) {
+  print(x);
+  ReaderMutexUnlock scope(&mu);
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+}
+
+void innerUnlock() {
+  MutexLock outer(&mu);
+  if (x == 0) {
+MutexUnlock inner(&mu);
+x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  }
+  x = 2;
+}
+
+void innerUnlockShared() {
+  ReaderMutexLock outer(&mu);
+  if (x == 0) {
+ReaderMutexUnlock inner(&mu);
+print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  }
+  print(x);
+}
+
+void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope(&mu);
+  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(&mu);
+  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(&mu);
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope(&mu);
+  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(&mu, &other);
+  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 &CE, SourceLocation Loc,
-  const CapExprSet &Excl, const CapExprSet &Shrd)
+

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

[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(&mu);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void simpleShared() SHARED_LOCKS_REQUIRED(mu) {
+  print(x);
+  ReaderMutexUnlock scope(&mu);
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+}
+
+void innerUnlock() {
+  MutexLock outer(&mu);
+  if (x == 0) {
+MutexUnlock inner(&mu);
+x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  }
+  x = 2;
+}
+
+void innerUnlockShared() {
+  ReaderMutexLock outer(&mu);
+  if (x == 0) {
+ReaderMutexUnlock inner(&mu);
+print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  }
+  print(x);
+}
+
+void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope(&mu);
+  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(&mu);
+  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(&mu);
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope(&mu);
+  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(&mu, &other);
+  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 &CE, SourceLocation Loc,
-  const CapExprSet &Excl, const CapExprSet &Shrd)
+  const CapExprSet &Excl, const CapExprSet &Shrd,
+  const CapExprSet &ExclRel, const CapExprSet &ShrdRel)
   : FactEntry(CE, LK_Exclusive, Loc, false) {
 for (const auto &M : Excl)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
 for (const auto &M : Shrd)
-  Unde

[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(&mu);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void simpleShared() SHARED_LOCKS_REQUIRED(mu) {
+  print(x);
+  ReaderMutexUnlock scope(&mu);
+  print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+}
+
+void innerUnlock() {
+  MutexLock outer(&mu);
+  if (x == 0) {
+MutexUnlock inner(&mu);
+x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  }
+  x = 2;
+}
+
+void innerUnlockShared() {
+  ReaderMutexLock outer(&mu);
+  if (x == 0) {
+ReaderMutexUnlock inner(&mu);
+print(x); // expected-warning {{reading variable 'x' requires holding mutex 'mu'}}
+  }
+  print(x);
+}
+
+void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope(&mu);
+  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(&mu);
+  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(&mu);
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope(&mu);
+  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(&mu, &other);
+  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 &CE, SourceLocation Loc,
-  const CapExprSet &Excl, const CapExprSet &Shrd)
+  const CapExprSet &Excl, const CapExprSet &Shrd,
+  const CapExprSet &ExclRel, const CapExprSet &ShrdRel)
   : FactEntry(CE, LK_Exclusive, Loc, false) {
 for (const auto &M : Excl)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
 for (const auto &M : Shrd)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
+for (const auto &M :

[PATCH] D56967: Thread safety analysis: Improve diagnostics for double locking

[Aaron Puchert via Phabricator via cfe-commits]

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

We use the existing diag::note_locked_here to tell the user where we saw
the first locking.


Repository:
  rC Clang

https://reviews.llvm.org/D56967

Files:
  include/clang/Analysis/Analyses/ThreadSafety.h
  lib/Analysis/ThreadSafety.cpp
  lib/Sema/AnalysisBasedWarnings.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
@@ -239,7 +239,7 @@
 }
 
 void sls_fun_bad_2() {
-  sls_mu.Lock();
+  sls_mu.Lock(); // expected-note{{mutex acquired here}}
   sls_mu.Lock(); // \
 // expected-warning{{acquiring mutex 'sls_mu' that is already held}}
   sls_mu.Unlock();
@@ -365,7 +365,7 @@
 }
 
 void aa_fun_bad_2() {
-  glock.globalLock();
+  glock.globalLock(); // expected-note{{mutex acquired here}}
   glock.globalLock(); // \
 // expected-warning{{acquiring mutex 'aa_mu' that is already held}}
   glock.globalUnlock();
@@ -1691,7 +1691,7 @@
   }
 
   void foo3() {
-MutexLock mulock_a(&mu1);
+MutexLock mulock_a(&mu1); // expected-note{{mutex acquired here}}
 MutexLock mulock_b(&mu1); // \
   // expected-warning {{acquiring mutex 'mu1' that is already held}}
   }
@@ -2710,14 +2710,14 @@
 }
 
 void doubleLock1() {
-  RelockableExclusiveMutexLock scope(&mu);
+  RelockableExclusiveMutexLock scope(&mu); // expected-note{{mutex acquired here}}
   scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
 }
 
 void doubleLock2() {
   RelockableExclusiveMutexLock scope(&mu);
   scope.Unlock();
-  scope.Lock();
+  scope.Lock(); // expected-note{{mutex acquired here}}
   scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
 }
 
@@ -2754,7 +2754,7 @@
 };
 
 void relockShared2() {
-  MemberLock lock;
+  MemberLock lock; // expected-note{{mutex acquired here}}
   lock.Lock(); // expected-warning {{acquiring mutex 'lock.mutex' that is already held}}
 }
 
@@ -2861,7 +2861,7 @@
 
 void doubleLock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
   MutexUnlock scope(&mu);
-  scope.Lock();
+  scope.Lock(); // expected-note{{mutex acquired here}}
   scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
 }
 
@@ -3164,7 +3164,7 @@
 
 
 void Foo::test8() {
-  mu_->Lock();
+  mu_->Lock();  // expected-note 2 {{mutex acquired here}}
   mu_.get()->Lock();// expected-warning {{acquiring mutex 'mu_' that is already held}}
   (*mu_).Lock();// expected-warning {{acquiring mutex 'mu_' that is already held}}
   mu_.get()->Unlock();
@@ -3298,7 +3298,7 @@
   foo.lock();
   foo.unlock();
 
-  foo.lock();
+  foo.lock(); // expected-note{{mutex acquired here}}
   foo.lock(); // expected-warning {{acquiring mutex 'foo' that is already held}}
   foo.unlock();
   foo.unlock();   // expected-warning {{releasing mutex 'foo' that was not held}}
@@ -3311,7 +3311,7 @@
   foo.a = 0;
   foo.unlock1();
 
-  foo.lock1();
+  foo.lock1();// expected-note{{mutex acquired here}}
   foo.lock1();// expected-warning {{acquiring mutex 'foo.mu1_' that is already held}}
   foo.a = 0;
   foo.unlock1();
@@ -3325,7 +3325,7 @@
   int d1 = foo.a;
   foo.unlock1();
 
-  foo.slock1();
+  foo.slock1();// expected-note{{mutex acquired here}}
   foo.slock1();// expected-warning {{acquiring mutex 'foo.mu1_' that is already held}}
   int d2 = foo.a;
   foo.unlock1();
@@ -3342,7 +3342,7 @@
   foo.c = 0;
   foo.unlock3();
 
-  foo.lock3();
+  foo.lock3(); // expected-note 3 {{mutex acquired here}}
   foo.lock3(); // \
 // expected-warning {{acquiring mutex 'foo.mu1_' that is already held}} \
 // expected-warning {{acquiring mutex 'foo.mu2_' that is already held}} \
@@ -3366,7 +3366,7 @@
   foo.c = 0;
   foo.unlocklots();
 
-  foo.locklots();
+  foo.locklots(); // expected-note 3 {{mutex acquired here}}
   foo.locklots(); // \
 // expected-warning {{acquiring mutex 'foo.mu1_' that is already held}} \
 // expected-warning {{acquiring mutex 'foo.mu2_' that is already held}} \
@@ -3524,7 +3524,7 @@
   LockAllGraphs();
   g2.mu_.Unlock();
 
-  LockAllGraphs();
+  LockAllGraphs(); // expected-note{{mutex acquired here}}
   g1.mu_.Lock();  // expected-warning {{acquiring mutex 'g1.mu_' that is already held}}
   g1.mu_.Unlock();
 }
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -1638,17 +1638,6 @@
 return ONS;
   }
 
-  // Helper functions
-  void warnLockMismatch(unsigned DiagID, StringRef Kind, Name LockName,
-SourceLocation Loc) {
-// Gracefully handle rare cases when the analysis can't get a more
-// precise source location.
-if (

[PATCH] D56967: Thread safety analysis: Improve diagnostics for double locking

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked 2 inline comments as done.
aaronpuchert added a comment.

Thanks for the review! I'll commit this when I have commit access again, which 
is waiting for my employer's approval to the relicensing.




Comment at: lib/Sema/AnalysisBasedWarnings.cpp:1693
+ << Kind << LockName);
+OptionalNotes notes =
+LocLocked.isValid()

I'll assume it's also `Notes` then. I tend to subconsciously forget these 
things.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56967



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

[PATCH] D56967: Thread safety analysis: Improve diagnostics for double locking

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352549: Thread safety analysis: Improve diagnostics for 
double locking (authored by aaronpuchert, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56967?vs=182687&id=184170#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56967

Files:
  cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
  cfe/trunk/lib/Analysis/ThreadSafety.cpp
  cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
  cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp

Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1638,17 +1638,6 @@
 return ONS;
   }
 
-  // Helper functions
-  void warnLockMismatch(unsigned DiagID, StringRef Kind, Name LockName,
-SourceLocation Loc) {
-// Gracefully handle rare cases when the analysis can't get a more
-// precise source location.
-if (!Loc.isValid())
-  Loc = FunLocation;
-PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << LockName);
-Warnings.emplace_back(std::move(Warning), getNotes());
-  }
-
  public:
   ThreadSafetyReporter(Sema &S, SourceLocation FL, SourceLocation FEL)
 : S(S), FunLocation(FL), FunEndLocation(FEL),
@@ -1677,7 +1666,11 @@
 
   void handleUnmatchedUnlock(StringRef Kind, Name LockName,
  SourceLocation Loc) override {
-warnLockMismatch(diag::warn_unlock_but_no_lock, Kind, LockName, Loc);
+if (Loc.isInvalid())
+  Loc = FunLocation;
+PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_but_no_lock)
+ << Kind << LockName);
+Warnings.emplace_back(std::move(Warning), getNotes());
   }
 
   void handleIncorrectUnlockKind(StringRef Kind, Name LockName,
@@ -1691,8 +1684,18 @@
 Warnings.emplace_back(std::move(Warning), getNotes());
   }
 
-  void handleDoubleLock(StringRef Kind, Name LockName, SourceLocation Loc) override {
-warnLockMismatch(diag::warn_double_lock, Kind, LockName, Loc);
+  void handleDoubleLock(StringRef Kind, Name LockName, SourceLocation LocLocked,
+SourceLocation Loc) override {
+if (Loc.isInvalid())
+  Loc = FunLocation;
+PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_double_lock)
+ << Kind << LockName);
+OptionalNotes Notes =
+LocLocked.isValid()
+? getNotes(PartialDiagnosticAt(
+  LocLocked, S.PDiag(diag::note_locked_here) << Kind))
+: getNotes();
+Warnings.emplace_back(std::move(Warning), std::move(Notes));
   }
 
   void handleMutexHeldEndOfScope(StringRef Kind, Name LockName,
Index: cfe/trunk/lib/Analysis/ThreadSafety.cpp
===
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp
@@ -873,7 +873,7 @@
   void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
   ThreadSafetyHandler &Handler,
   StringRef DiagKind) const override {
-Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc());
+Handler.handleDoubleLock(DiagKind, entry.toString(), loc(), entry.loc());
   }
 
   void handleUnlock(FactSet &FSet, FactManager &FactMan,
@@ -981,12 +981,13 @@
   void lock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
 LockKind kind, SourceLocation loc, ThreadSafetyHandler *Handler,
 StringRef DiagKind) const {
-if (!FSet.findLock(FactMan, Cp)) {
+if (const FactEntry *Fact = FSet.findLock(FactMan, Cp)) {
+  if (Handler)
+Handler->handleDoubleLock(DiagKind, Cp.toString(), Fact->loc(), loc);
+} else {
   FSet.removeLock(FactMan, !Cp);
   FSet.addLock(FactMan,
llvm::make_unique(Cp, kind, loc));
-} else if (Handler) {
-  Handler->handleDoubleLock(DiagKind, Cp.toString(), loc);
 }
   }
 
Index: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
===
--- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
+++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -128,9 +128,10 @@
   /// \param Kind -- the capability's name parameter (role, mutex, etc).
   /// \param LockName -- A StringRef name for the lock expression, to be printed
   /// in the error message.
+  /// \param LocLocked -- The location of the first lock expression.
   /// \param Loc -- The location of the second lock expression.
   virtual void handleDoubleLock(StringRef Kind, Name LockName,
-SourceLocation Loc) {}
+Sour

[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] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: bkramer, efriedma, rsmith.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

I've found that most often the proper way to fix this warning is to add
`static`, because if the code otherwise compiles and links, the function
or variable is apparently not needed outside of the TU.

There is no fix-it for the rare case of an "extern definition", because
that would require removing `extern` and I have no idea how to get the
source location of the storage class specifier from a VarDecl. I believe
this information is only available earlier in the AST construction from
DeclSpec::getStorageClassSpecLoc(), but we don't have that here.


Repository:
  rC Clang

https://reviews.llvm.org/D59402

Files:
  lib/Sema/SemaDecl.cpp
  test/Sema/warn-missing-prototypes.c
  test/Sema/warn-missing-variable-declarations.c
  test/SemaCXX/warn-missing-variable-declarations.cpp

Index: test/SemaCXX/warn-missing-variable-declarations.cpp
===
--- test/SemaCXX/warn-missing-variable-declarations.cpp
+++ test/SemaCXX/warn-missing-variable-declarations.cpp
@@ -1,11 +1,15 @@
-// RUN: %clang -Wmissing-variable-declarations -fsyntax-only -Xclang -verify -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-variable-declarations -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -Wmissing-variable-declarations -fdiagnostics-parseable-fixits -std=c++17 %s 2>&1 | FileCheck %s
 
 // Variable declarations that should trigger a warning.
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 namespace x {
   int vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:3}:"static "
 }
 
 // Variable declarations that should not trigger a warning.
@@ -58,7 +62,9 @@
 constexpr int constexpr_var = 0;
 inline constexpr int inline_constexpr_var = 0;
 extern const int extern_const_var = 0; // expected-warning {{no previous extern declaration}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 extern constexpr int extern_constexpr_var = 0; // expected-warning {{no previous extern declaration}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 
 template int var_template = 0;
 template constexpr int const_var_template = 0;
Index: test/Sema/warn-missing-variable-declarations.c
===
--- test/Sema/warn-missing-variable-declarations.c
+++ test/Sema/warn-missing-variable-declarations.c
@@ -1,6 +1,8 @@
 // RUN: %clang_cc1 -Wmissing-variable-declarations -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wmissing-variable-declarations -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 int vbad2;
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
@@ -8,6 +10,7 @@
 struct {
   int mgood1;
 } vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:1-[[@LINE-3]]:1}:"static "
 
 int vbad4;
 int vbad4 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad4'}}
Index: test/Sema/warn-missing-prototypes.c
===
--- test/Sema/warn-missing-prototypes.c
+++ test/Sema/warn-missing-prototypes.c
@@ -4,6 +4,7 @@
 int f();
 
 int f(int x) { return x; } // expected-warning{{no previous prototype for function 'f'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 static int g(int x) { return x; }
 
@@ -13,6 +14,9 @@
 
 int g2(int x) { return x; }
 
+extern int g3(int x) { return x; } // expected-warning{{no previous prototype for function 'g3'}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
+
 void test(void);
 
 int h3();
@@ -40,4 +44,4 @@
 void not_a_prototype_test(); // expected-note{{this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function}}
 void not_a_prototype_test() { } // expected-warning{{no previous prototype for function 'not_a_prototype_test'}}
 
-// CHECK: fix-it:"{{.*}}":{40:27-40:27}:"void"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:27-[[@LINE-3]]:27}:"void"
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11805,7 +11805,11 @@
   prev = prev->getPreviousDecl();
 
 if 

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: test/Sema/warn-missing-prototypes.c:7
 int f(int x) { return x; } // expected-warning{{no previous prototype for 
function 'f'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 

Maybe there shouldn't be a fix-it here, as there is a declaration, which is 
just not a prototype.



Comment at: test/Sema/warn-missing-prototypes.c:17-18
 
+extern int g3(int x) { return x; } // expected-warning{{no previous prototype 
for function 'g3'}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
+

As I wrote in the commit message: this could be fixed by replacing `extern` 
with `static`, but I don't think we have the `SourceLocation` for the storage 
class specifier available where the warning is emitted. Maybe I'm overlooking 
something though.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59402



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

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

[Aaron Puchert via Phabricator via cfe-commits]

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

Similar to D56967 , we add the existing 
diag::note_locked_here to tell
the user where we saw the locking that isn't matched correctly.


Repository:
  rC Clang

https://reviews.llvm.org/D59455

Files:
  include/clang/Analysis/Analyses/ThreadSafety.h
  lib/Analysis/ThreadSafety.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  test/Sema/warn-thread-safety-analysis.c
  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
@@ -726,26 +726,26 @@
 }
 
 void shared_bad_3() {
-  sls_mu.Lock();
+  sls_mu.Lock(); // expected-note {{mutex acquired here}}
   sls_mu.ReaderUnlock(); // \
 // expected-warning {{releasing mutex 'sls_mu' using shared access, expected exclusive access}}
 }
 
 void shared_bad_4() {
-  sls_mu.ReaderLock();
+  sls_mu.ReaderLock();  // expected-note {{mutex acquired here}}
   sls_mu.ExclusiveUnlock(); // \
 // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
 }
 
 void shared_bad_5() {
-  sls_mu.Lock();
+  sls_mu.Lock();  // expected-note {{mutex acquired here}}
   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.ReaderLock();  // expected-note {{mutex acquired here}}
   sls_mu.DemoteExclusive(); // \
 // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
   sls_mu.ReaderUnlock();
Index: test/Sema/warn-thread-safety-analysis.c
===
--- test/Sema/warn-thread-safety-analysis.c
+++ test/Sema/warn-thread-safety-analysis.c
@@ -117,11 +117,11 @@
   (void)(*d_ == 1);
   mutex_unlock(foo_.mu_);
 
-  mutex_exclusive_lock(&mu1);
+  mutex_exclusive_lock(&mu1);// expected-note {{mutex acquired here}}
   mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using shared access, expected exclusive access}}
   mutex_exclusive_unlock(&mu1);  // expected-warning {{releasing mutex 'mu1' that was not held}}
 
-  mutex_shared_lock(&mu1);
+  mutex_shared_lock(&mu1);  // expected-note {{mutex acquired here}}
   mutex_exclusive_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using exclusive access, expected shared access}}
   mutex_shared_unlock(&mu1);// expected-warning {{releasing mutex 'mu1' that was not held}}
 
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -1642,6 +1642,13 @@
 return ONS;
   }
 
+  OptionalNotes makeLockedHereNote(SourceLocation LocLocked, StringRef Kind) {
+return LocLocked.isValid()
+   ? getNotes(PartialDiagnosticAt(
+ LocLocked, S.PDiag(diag::note_locked_here) << Kind))
+   : getNotes();
+  }
+
  public:
   ThreadSafetyReporter(Sema &S, SourceLocation FL, SourceLocation FEL)
 : S(S), FunLocation(FL), FunEndLocation(FEL),
@@ -1679,13 +1686,15 @@
 
   void handleIncorrectUnlockKind(StringRef Kind, Name LockName,
  LockKind Expected, LockKind Received,
+ SourceLocation LocLocked,
  SourceLocation Loc) override {
 if (Loc.isInvalid())
   Loc = FunLocation;
 PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_kind_mismatch)
  << Kind << LockName << Received
  << Expected);
-Warnings.emplace_back(std::move(Warning), getNotes());
+Warnings.emplace_back(std::move(Warning),
+  makeLockedHereNote(LocLocked, Kind));
   }
 
   void handleDoubleLock(StringRef Kind, Name LockName, SourceLocation LocLocked,
@@ -1694,12 +1703,8 @@
   Loc = FunLocation;
 PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_double_lock)
  << Kind << LockName);
-OptionalNotes Notes =
-LocLocked.isValid()
-? getNotes(PartialDiagnosticAt(
-  LocLocked, S.PDiag(diag::note_locked_here) << Kind))
-: getNotes();
-Warnings.emplace_back(std::move(Warning), std::move(Notes));
+Warnings.emplace_back(std::move(Warning),
+  makeLockedHereNote(LocLocked, Kind));
   }
 
   void handleMutexHeldEndOfScope(StringRef Kind, Name LockName,
Index: lib/Analysis/ThreadSafety.cpp
=

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

[Aaron Puchert via Phabricator via cfe-commits]

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

Don't suggest adding `static` if there is a non-prototype declaration. This 
required a minor refactoring: we let `ShouldWarnAboutMissingPrototype` return 
any kind of declaration it finds and check for the number of parameters at the 
call site.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59402

Files:
  lib/Sema/SemaDecl.cpp
  test/Sema/warn-missing-prototypes.c
  test/Sema/warn-missing-variable-declarations.c
  test/SemaCXX/warn-missing-variable-declarations.cpp

Index: test/SemaCXX/warn-missing-variable-declarations.cpp
===
--- test/SemaCXX/warn-missing-variable-declarations.cpp
+++ test/SemaCXX/warn-missing-variable-declarations.cpp
@@ -1,11 +1,15 @@
-// RUN: %clang -Wmissing-variable-declarations -fsyntax-only -Xclang -verify -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-variable-declarations -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -Wmissing-variable-declarations -fdiagnostics-parseable-fixits -std=c++17 %s 2>&1 | FileCheck %s
 
 // Variable declarations that should trigger a warning.
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 namespace x {
   int vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:3}:"static "
 }
 
 // Variable declarations that should not trigger a warning.
@@ -58,7 +62,9 @@
 constexpr int constexpr_var = 0;
 inline constexpr int inline_constexpr_var = 0;
 extern const int extern_const_var = 0; // expected-warning {{no previous extern declaration}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 extern constexpr int extern_constexpr_var = 0; // expected-warning {{no previous extern declaration}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 
 template int var_template = 0;
 template constexpr int const_var_template = 0;
Index: test/Sema/warn-missing-variable-declarations.c
===
--- test/Sema/warn-missing-variable-declarations.c
+++ test/Sema/warn-missing-variable-declarations.c
@@ -1,6 +1,8 @@
 // RUN: %clang_cc1 -Wmissing-variable-declarations -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wmissing-variable-declarations -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 int vbad2;
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
@@ -8,6 +10,7 @@
 struct {
   int mgood1;
 } vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:1-[[@LINE-3]]:1}:"static "
 
 int vbad4;
 int vbad4 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad4'}}
Index: test/Sema/warn-missing-prototypes.c
===
--- test/Sema/warn-missing-prototypes.c
+++ test/Sema/warn-missing-prototypes.c
@@ -4,15 +4,20 @@
 int f();
 
 int f(int x) { return x; } // expected-warning{{no previous prototype for function 'f'}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 
 static int g(int x) { return x; }
 
 int h(int x) { return x; } // expected-warning{{no previous prototype for function 'h'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 static int g2();
 
 int g2(int x) { return x; }
 
+extern int g3(int x) { return x; } // expected-warning{{no previous prototype for function 'g3'}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
+
 void test(void);
 
 int h3();
@@ -40,4 +45,4 @@
 void not_a_prototype_test(); // expected-note{{this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function}}
 void not_a_prototype_test() { } // expected-warning{{no previous prototype for function 'not_a_prototype_test'}}
 
-// CHECK: fix-it:"{{.*}}":{40:27-40:27}:"void"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:27-[[@LINE-3]]:27}:"void"
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11805,7 +11805,11 @@
   prev = prev->getPreviousDecl();
 
 if (!prev)
-  Diag(var->getLocation(), diag::warn_missing_variable_declarations) << var;
+  Diag(var->getLocation(), diag::warn_missing_variable_declarations)
+  << var

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

[Aaron Puchert via Phabricator via cfe-commits]

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

Factor out some common code.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59455

Files:
  include/clang/Analysis/Analyses/ThreadSafety.h
  lib/Analysis/ThreadSafety.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  test/Sema/warn-thread-safety-analysis.c
  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
@@ -726,26 +726,26 @@
 }
 
 void shared_bad_3() {
-  sls_mu.Lock();
+  sls_mu.Lock(); // expected-note {{mutex acquired here}}
   sls_mu.ReaderUnlock(); // \
 // expected-warning {{releasing mutex 'sls_mu' using shared access, expected exclusive access}}
 }
 
 void shared_bad_4() {
-  sls_mu.ReaderLock();
+  sls_mu.ReaderLock();  // expected-note {{mutex acquired here}}
   sls_mu.ExclusiveUnlock(); // \
 // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
 }
 
 void shared_bad_5() {
-  sls_mu.Lock();
+  sls_mu.Lock();  // expected-note {{mutex acquired here}}
   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.ReaderLock();  // expected-note {{mutex acquired here}}
   sls_mu.DemoteExclusive(); // \
 // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
   sls_mu.ReaderUnlock();
Index: test/Sema/warn-thread-safety-analysis.c
===
--- test/Sema/warn-thread-safety-analysis.c
+++ test/Sema/warn-thread-safety-analysis.c
@@ -117,11 +117,11 @@
   (void)(*d_ == 1);
   mutex_unlock(foo_.mu_);
 
-  mutex_exclusive_lock(&mu1);
+  mutex_exclusive_lock(&mu1);// expected-note {{mutex acquired here}}
   mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using shared access, expected exclusive access}}
   mutex_exclusive_unlock(&mu1);  // expected-warning {{releasing mutex 'mu1' that was not held}}
 
-  mutex_shared_lock(&mu1);
+  mutex_shared_lock(&mu1);  // expected-note {{mutex acquired here}}
   mutex_exclusive_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using exclusive access, expected shared access}}
   mutex_shared_unlock(&mu1);// expected-warning {{releasing mutex 'mu1' that was not held}}
 
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -1642,6 +1642,13 @@
 return ONS;
   }
 
+  OptionalNotes makeLockedHereNote(SourceLocation LocLocked, StringRef Kind) {
+return LocLocked.isValid()
+   ? getNotes(PartialDiagnosticAt(
+ LocLocked, S.PDiag(diag::note_locked_here) << Kind))
+   : getNotes();
+  }
+
  public:
   ThreadSafetyReporter(Sema &S, SourceLocation FL, SourceLocation FEL)
 : S(S), FunLocation(FL), FunEndLocation(FEL),
@@ -1679,13 +1686,15 @@
 
   void handleIncorrectUnlockKind(StringRef Kind, Name LockName,
  LockKind Expected, LockKind Received,
+ SourceLocation LocLocked,
  SourceLocation Loc) override {
 if (Loc.isInvalid())
   Loc = FunLocation;
 PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_kind_mismatch)
  << Kind << LockName << Received
  << Expected);
-Warnings.emplace_back(std::move(Warning), getNotes());
+Warnings.emplace_back(std::move(Warning),
+  makeLockedHereNote(LocLocked, Kind));
   }
 
   void handleDoubleLock(StringRef Kind, Name LockName, SourceLocation LocLocked,
@@ -1694,12 +1703,8 @@
   Loc = FunLocation;
 PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_double_lock)
  << Kind << LockName);
-OptionalNotes Notes =
-LocLocked.isValid()
-? getNotes(PartialDiagnosticAt(
-  LocLocked, S.PDiag(diag::note_locked_here) << Kind))
-: getNotes();
-Warnings.emplace_back(std::move(Warning), std::move(Notes));
+Warnings.emplace_back(std::move(Warning),
+  makeLockedHereNote(LocLocked, Kind));
   }
 
   void handleMutexHeldEndOfScope(StringRef Kind, Name LockName,
@@ -1726,13 +1731,8 @@
 
 PartialDiagnosticAt Warning(LocEndOfScope, S.PDiag(DiagID) << Kind
<< LockName);
-if (LocLocked.isValid()) {
-  PartialDiagnost

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: include/clang/Analysis/Analyses/ThreadSafety.h:127
+ SourceLocation LocLocked,
  SourceLocation Loc) {}
 

aaron.ballman wrote:
> Can you rename this one to `LocUnlocked` to mirror the new parameter?
This was supposed to mirror the following function (handleDoubleLock), where 
similarly `Loc` is the place of the issue itself and `LocLocked` indicates 
where the lock is coming from. If we want to change this, I would suggest to 
use present tense here: `LocUnlock`. The lock from `LocLocked` "happened in the 
past" (meaning earlier in the code), but the unlock "happens now" (meaning at 
the current location).


Repository:
  rC Clang

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

https://reviews.llvm.org/D59455



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

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356427: Thread safety analysis: Add note for unlock kind 
mismatch (authored by aaronpuchert, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59455?vs=190968&id=191207#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59455

Files:
  include/clang/Analysis/Analyses/ThreadSafety.h
  lib/Analysis/ThreadSafety.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  test/Sema/warn-thread-safety-analysis.c
  test/SemaCXX/warn-thread-safety-analysis.cpp

Index: include/clang/Analysis/Analyses/ThreadSafety.h
===
--- include/clang/Analysis/Analyses/ThreadSafety.h
+++ include/clang/Analysis/Analyses/ThreadSafety.h
@@ -119,9 +119,11 @@
   /// \param Kind -- the capability's name parameter (role, mutex, etc).
   /// \param Expected -- the kind of lock expected.
   /// \param Received -- the kind of lock received.
+  /// \param LocLocked -- The SourceLocation of the Lock.
   /// \param Loc -- The SourceLocation of the Unlock.
   virtual void handleIncorrectUnlockKind(StringRef Kind, Name LockName,
  LockKind Expected, LockKind Received,
+ SourceLocation LocLocked,
  SourceLocation Loc) {}
 
   /// Warn about lock function calls for locks which are already held.
Index: test/Sema/warn-thread-safety-analysis.c
===
--- test/Sema/warn-thread-safety-analysis.c
+++ test/Sema/warn-thread-safety-analysis.c
@@ -117,11 +117,11 @@
   (void)(*d_ == 1);
   mutex_unlock(foo_.mu_);
 
-  mutex_exclusive_lock(&mu1);
+  mutex_exclusive_lock(&mu1);// expected-note {{mutex acquired here}}
   mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using shared access, expected exclusive access}}
   mutex_exclusive_unlock(&mu1);  // expected-warning {{releasing mutex 'mu1' that was not held}}
 
-  mutex_shared_lock(&mu1);
+  mutex_shared_lock(&mu1);  // expected-note {{mutex acquired here}}
   mutex_exclusive_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using exclusive access, expected shared access}}
   mutex_shared_unlock(&mu1);// expected-warning {{releasing mutex 'mu1' that was not held}}
 
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -726,26 +726,26 @@
 }
 
 void shared_bad_3() {
-  sls_mu.Lock();
+  sls_mu.Lock(); // expected-note {{mutex acquired here}}
   sls_mu.ReaderUnlock(); // \
 // expected-warning {{releasing mutex 'sls_mu' using shared access, expected exclusive access}}
 }
 
 void shared_bad_4() {
-  sls_mu.ReaderLock();
+  sls_mu.ReaderLock();  // expected-note {{mutex acquired here}}
   sls_mu.ExclusiveUnlock(); // \
 // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
 }
 
 void shared_bad_5() {
-  sls_mu.Lock();
+  sls_mu.Lock();  // expected-note {{mutex acquired here}}
   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.ReaderLock();  // expected-note {{mutex acquired here}}
   sls_mu.DemoteExclusive(); // \
 // expected-warning {{releasing mutex 'sls_mu' using exclusive access, expected shared access}}
   sls_mu.ReaderUnlock();
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -1642,6 +1642,13 @@
 return ONS;
   }
 
+  OptionalNotes makeLockedHereNote(SourceLocation LocLocked, StringRef Kind) {
+return LocLocked.isValid()
+   ? getNotes(PartialDiagnosticAt(
+ LocLocked, S.PDiag(diag::note_locked_here) << Kind))
+   : getNotes();
+  }
+
  public:
   ThreadSafetyReporter(Sema &S, SourceLocation FL, SourceLocation FEL)
 : S(S), FunLocation(FL), FunEndLocation(FEL),
@@ -1679,13 +1686,15 @@
 
   void handleIncorrectUnlockKind(StringRef Kind, Name LockName,
  LockKind Expected, LockKind Received,
+ SourceLocation LocLocked,
  SourceLocation Loc) override {
 if (Loc.isInvalid())
   Loc = FunLocation;
 PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_kind_mismatch)
  << Kind << LockName << Received
  << Expected);
-Warnings.emplace_back(std::move(Warning), ge

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked 4 inline comments as done.
aaronpuchert added inline comments.



Comment at: include/clang/Analysis/Analyses/ThreadSafety.h:127
+ SourceLocation LocLocked,
  SourceLocation Loc) {}
 

aaron.ballman wrote:
> aaronpuchert wrote:
> > aaron.ballman wrote:
> > > Can you rename this one to `LocUnlocked` to mirror the new parameter?
> > This was supposed to mirror the following function (handleDoubleLock), 
> > where similarly `Loc` is the place of the issue itself and `LocLocked` 
> > indicates where the lock is coming from. If we want to change this, I would 
> > suggest to use present tense here: `LocUnlock`. The lock from `LocLocked` 
> > "happened in the past" (meaning earlier in the code), but the unlock 
> > "happens now" (meaning at the current location).
> Ah, good call on the tense of the identifier! Given that it's following a 
> pattern, I'd say you can commit as-is. If you're similarly bothered by the 
> name, then you could have a follow-up NFC commit that renames consistently 
> across APIs.
Changed in rC356430.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59455



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

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

> It should consider both because the attributes can be used on Objective-C as 
> well.

Well, it's not really supported that well though. There are known bugs like 
https://bugs.llvm.org/show_bug.cgi?id=38892, and I don't really have the time 
to fix that. (You're free to jump in of course.)

> It seems weird how both do pretty similar things and e.g. duplicate 
> getParamDecl.

I have no idea about ObjC at all, but I believe that ObjC methods are 
substantially different from ordinary functions. (Unlike C++ methods, which 
just have an additional implicit parameter.)

You could do something like `SExprBuilder::enterCFG`:

  auto Parms = isa(D) ? cast(D)->parameters()
  : cast(D)->parameters();



> The repro I have is 20M, creduce chokes on it, and manually reducing failed 
> to repro... ☹️

You could try declaring an ObjC method with a thread safety attribute that 
refers to a parameter, then somewhere else call that function. Again, I don't 
know ObjC, but for a C function you would do:

  struct Data { Mutex mu; }
  void f(struct Data *data) __attribute__ 
((exclusive_locks_required(data->mu)));
  
  void g() {
Data d;
Lock(&d.mu);
f(&d);  // Should only work with your change.
Unlock(&d.mu);
  }

Then the SExprBuilder should have to replace the reference to the parameter 
(here `data`) by the value it is called with (here `&d`). I'll have to admit I 
don't find it easy to wrap my head around this logic either, so maybe I'm 
missing something. The tests for ObjC[++] are very brief, but you can look at 
the C++ tests for hints. (See 
`test/Sema{ObjC,ObjC++,C++}/warn-thread-safety-analysis.{m,mm,cpp}`.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523



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

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a reviewer: aaronpuchert.
aaronpuchert added inline comments.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282-285
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {

Unrelated to your change, but I'm wondering if this is right. `Ctx->AttrDecl` 
is a `NamedDecl`, on which `getCanonicalDecl()` just returns `*this` (so we 
could in fact omit it here without functional change), while on `FunctionDecl` 
and `ObjCMethodDecl` it does something non-trivial instead.

I guess I need to look into this a bit, don't let this bother you. But I think 
we might actually have to do the cast on both sides of the equation to get the 
same kind of canonical declaration. Or we make sure that `Ctx->AttrDecl` is 
already canonical.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

Test is fine for me, but I would like if you could integrate it with the 
existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread safety 
analysis requires a bit of setup, that's why we tend to keep the tests in one 
file. I'll admit that the C++ tests have grown quite large, but for ObjC++ it's 
still very manageable. 



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:10
+
+template  struct __attribute__((scoped_lockable)) Locker {
+  T &_l;

Can we hard-code `T` = `MyLock`? We can still change it if we need it more 
general later.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523



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

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:283-285
+  if (isa(D)
+  ? (cast(D)->getCanonicalDecl() == Canonical)
+  : (cast(D)->getCanonicalDecl() == Canonical)) {

aaron.ballman wrote:
> Also somewhat orthogonal to your changes, but... ugh, the lack of any common 
> base between `FunctionDecl` and `ObjcMethodDecl` strikes again. I sort of 
> wish we would introduce a CallableDecl base class that these two (and 
> BlockDecl) could all inherit from to take care of this sort of ugly hack.
Seems I was wrong, because `getCanonicalDecl()` is actually virtual. I was 
confused by the different return type, apparently an overriding function 
doesn't need to have the same return type. So no problem here.

This doesn't make our case easier: both `FunctionDecl` and `ObjCMethodDecl` 
inherit from both `Decl` and `DeclContext`, but independently. So we can't 
directly cast from `DeclContext` to `Decl`, we need to know which leaf we are.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

jfb wrote:
> aaronpuchert wrote:
> > Test is fine for me, but I would like if you could integrate it with the 
> > existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread safety 
> > analysis requires a bit of setup, that's why we tend to keep the tests in 
> > one file. I'll admit that the C++ tests have grown quite large, but for 
> > ObjC++ it's still very manageable. 
> Sure thing! I created a header that's shared and simplified this repro a bit. 
> I validated that the shared code was crashing before and this fixes the crash.
I would still like all ObjCXX tests for -Wthread-safety in one file, because 
that's what we have for the other languages as well. (more or less)

It's somewhat easier to see which aspects are tested then and which are not. 
Right now we only have tests for reported crashes, but maybe someday someone 
finds the time to test a bit more exhaustively.

But perhaps @aaron.ballman sees this another way, in that case follow his 
opinion.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: dblaikie, echristo.
Herald added subscribers: cfe-commits, jdoerfert, aprantl.
Herald added a project: clang.

With Split DWARF the resulting object file (then called skeleton CU)
contains the file name of another ("DWO") file with the debug info.
This is a problem for remote compilation, as the file name written there
will be that of the compilation server, not of the client.

To reliably enable Split DWARF in the case of of remote compilation, we
need the ability to inject arbitrary file names into this field, which
is now provided by the option

  -fsplit-dwarf-dwo-name-attr=

For now we keep this as CC1 option, but the idea is to promote it
eventually, if it picks up.


Repository:
  rC Clang

https://reviews.llvm.org/D59673

Files:
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-dwo-name-attr.c


Index: test/CodeGen/split-debug-dwo-name-attr.c
===
--- /dev/null
+++ test/CodeGen/split-debug-dwo-name-attr.c
@@ -0,0 +1,8 @@
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited 
-enable-split-dwarf -fdebug-compilation-dir /foo 
-fsplit-dwarf-dwo-name-attr=bar.dwo -split-dwarf-file %t -emit-obj -o - %s | 
llvm-objdump -s -section=.debug_str - | FileCheck %s
+// RUN: llvm-objdump -section-headers %t | FileCheck --check-prefix=DWO %s
+
+int f() { return 0; }
+
+// CHECK:  2f666f6f 00626172 2e64776f 00/foo.bar.dwo.
+// DWO: .dwo
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -714,6 +714,8 @@
   Opts.WholeProgramVTables = Args.hasArg(OPT_fwhole_program_vtables);
   Opts.LTOVisibilityPublicStd = Args.hasArg(OPT_flto_visibility_public_std);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
+  Opts.SplitDwarfDwoNameAttr =
+  Args.getLastArgValue(OPT_fsplit_dwarf_dwo_name_attr_EQ);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
 
   if (Arg *A =
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -464,7 +464,9 @@
   Options.EmitAddrsig = CodeGenOpts.Addrsig;
 
   if (CodeGenOpts.getSplitDwarfMode() != CodeGenOptions::NoFission)
-Options.MCOptions.SplitDwarfFile = CodeGenOpts.SplitDwarfFile;
+Options.MCOptions.SplitDwarfFile = 
CodeGenOpts.SplitDwarfDwoNameAttr.empty()
+   ? CodeGenOpts.SplitDwarfFile
+   : CodeGenOpts.SplitDwarfDwoNameAttr;
   Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
   Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
   Options.MCOptions.MCUseDwarfDirectory = !CodeGenOpts.NoDwarfDirectoryAsm;
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1849,6 +1849,8 @@
   Flags<[CC1Option]>, HelpText<"Use DWARF base address selection entries in 
debug_ranges">;
 def fno_debug_ranges_base_address: Flag <["-"], 
"fno-debug-ranges-base-address">, Group,
   Flags<[CC1Option]>;
+def fsplit_dwarf_dwo_name_attr_EQ: Joined<["-"], 
"fsplit-dwarf-dwo-name-attr=">, Group,
+  Flags<[CC1Option]>, HelpText<"Set the name for the split debug info file to 
be used in the object file">;
 def fsplit_dwarf_inlining: Flag <["-"], "fsplit-dwarf-inlining">, 
Group,
   Flags<[CC1Option]>, HelpText<"Provide minimal debug info in the 
object/executable to facilitate online symbolication/stack traces in the 
absence of .dwo/.dwp files when using Split DWARF">;
 def fno_split_dwarf_inlining: Flag<["-"], "fno-split-dwarf-inlining">, 
Group,
Index: include/clang/Basic/CodeGenOptions.h
===
--- include/clang/Basic/CodeGenOptions.h
+++ include/clang/Basic/CodeGenOptions.h
@@ -188,6 +188,10 @@
   /// in the backend for setting the name in the skeleton cu.
   std::string SplitDwarfFile;
 
+  /// Overrides \ref SplitDwarfFile as value of DW_AT_[GNU_]dwo_name to be used
+  /// in the skeleton CU, if not empty. Does not change the output file name.
+  std::string SplitDwarfDwoNameAttr;
+
   /// The name of the relocation model to use.
   llvm::Reloc::Model RelocationModel;
 


Index: test/CodeGen/split-debug-dwo-name-attr.c
===
--- /dev/null
+++ test/CodeGen/split-debug-dwo-name-attr.c
@@ -0,0 +1,8 @@
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -enable-split-dwarf -fdebug-compilation-dir /

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D59673#1438716 , @dblaikie wrote:

> Use llvm-dwarfdump rather than llvm-objdump to dump the contents of the 
> debug_info section and test the dwo_name there (rather than dumping hex)


I didn't know about llvm-dwarfdump, I wondered why llvm-objdump wouldn't 
pretty-print the debug info as objdump does. That makes a lot of sense then.

> probably the objdump part of the test isn't needed?

Actually I only need to check that the DWO file is there (under the proper file 
name). Any ideas?

> (& I guess there's an LLVM patch to add the rest of this functionality?)

What do we have to do there? Should we add the option to `llc` as well?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[Aaron Puchert via Phabricator via cfe-commits]

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

Use llvm-dwarfdump to inspect debug info, remove unneeded flags.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673

Files:
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-dwo-name-attr.c


Index: test/CodeGen/split-debug-dwo-name-attr.c
===
--- /dev/null
+++ test/CodeGen/split-debug-dwo-name-attr.c
@@ -0,0 +1,8 @@
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited 
-enable-split-dwarf -split-dwarf-file %t -fsplit-dwarf-dwo-name-attr=foo.dwo 
-emit-obj -o - %s | llvm-dwarfdump -debug-info - | FileCheck %s
+// RUN: llvm-objdump -section-headers %t | FileCheck --check-prefix=DWO %s
+
+int f() { return 0; }
+
+// CHECK: DW_AT_GNU_dwo_name ("foo.dwo")
+// DWO: .dwo
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -714,6 +714,8 @@
   Opts.WholeProgramVTables = Args.hasArg(OPT_fwhole_program_vtables);
   Opts.LTOVisibilityPublicStd = Args.hasArg(OPT_flto_visibility_public_std);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
+  Opts.SplitDwarfDwoNameAttr =
+  Args.getLastArgValue(OPT_fsplit_dwarf_dwo_name_attr_EQ);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
 
   if (Arg *A =
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -464,7 +464,9 @@
   Options.EmitAddrsig = CodeGenOpts.Addrsig;
 
   if (CodeGenOpts.getSplitDwarfMode() != CodeGenOptions::NoFission)
-Options.MCOptions.SplitDwarfFile = CodeGenOpts.SplitDwarfFile;
+Options.MCOptions.SplitDwarfFile = 
CodeGenOpts.SplitDwarfDwoNameAttr.empty()
+   ? CodeGenOpts.SplitDwarfFile
+   : CodeGenOpts.SplitDwarfDwoNameAttr;
   Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
   Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
   Options.MCOptions.MCUseDwarfDirectory = !CodeGenOpts.NoDwarfDirectoryAsm;
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1849,6 +1849,8 @@
   Flags<[CC1Option]>, HelpText<"Use DWARF base address selection entries in 
debug_ranges">;
 def fno_debug_ranges_base_address: Flag <["-"], 
"fno-debug-ranges-base-address">, Group,
   Flags<[CC1Option]>;
+def fsplit_dwarf_dwo_name_attr_EQ: Joined<["-"], 
"fsplit-dwarf-dwo-name-attr=">, Group,
+  Flags<[CC1Option]>, HelpText<"Set the name for the split debug info file to 
be used in the object file">;
 def fsplit_dwarf_inlining: Flag <["-"], "fsplit-dwarf-inlining">, 
Group,
   Flags<[CC1Option]>, HelpText<"Provide minimal debug info in the 
object/executable to facilitate online symbolication/stack traces in the 
absence of .dwo/.dwp files when using Split DWARF">;
 def fno_split_dwarf_inlining: Flag<["-"], "fno-split-dwarf-inlining">, 
Group,
Index: include/clang/Basic/CodeGenOptions.h
===
--- include/clang/Basic/CodeGenOptions.h
+++ include/clang/Basic/CodeGenOptions.h
@@ -188,6 +188,10 @@
   /// in the backend for setting the name in the skeleton cu.
   std::string SplitDwarfFile;
 
+  /// Overrides \ref SplitDwarfFile as value of DW_AT_[GNU_]dwo_name to be used
+  /// in the skeleton CU, if not empty. Does not change the output file name.
+  std::string SplitDwarfDwoNameAttr;
+
   /// The name of the relocation model to use.
   llvm::Reloc::Model RelocationModel;
 


Index: test/CodeGen/split-debug-dwo-name-attr.c
===
--- /dev/null
+++ test/CodeGen/split-debug-dwo-name-attr.c
@@ -0,0 +1,8 @@
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -enable-split-dwarf -split-dwarf-file %t -fsplit-dwarf-dwo-name-attr=foo.dwo -emit-obj -o - %s | llvm-dwarfdump -debug-info - | FileCheck %s
+// RUN: llvm-objdump -section-headers %t | FileCheck --check-prefix=DWO %s
+
+int f() { return 0; }
+
+// CHECK: DW_AT_GNU_dwo_name ("foo.dwo")
+// DWO: .dwo
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -714,6 +714,8 @@
   Opts.WholeProgramVTables = Args.hasArg(OPT_fwhole_program_vtables);
   Opts.LTOVisibilityPublicStd = Args.hasArg(OPT_flto_visibil

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class 
%s
+

aaron.ballman wrote:
> jfb wrote:
> > aaron.ballman wrote:
> > > jfb wrote:
> > > > aaron.ballman wrote:
> > > > > aaronpuchert wrote:
> > > > > > jfb wrote:
> > > > > > > aaronpuchert wrote:
> > > > > > > > Test is fine for me, but I would like if you could integrate it 
> > > > > > > > with the existing 
> > > > > > > > test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread 
> > > > > > > > safety analysis requires a bit of setup, that's why we tend to 
> > > > > > > > keep the tests in one file. I'll admit that the C++ tests have 
> > > > > > > > grown quite large, but for ObjC++ it's still very manageable. 
> > > > > > > Sure thing! I created a header that's shared and simplified this 
> > > > > > > repro a bit. I validated that the shared code was crashing before 
> > > > > > > and this fixes the crash.
> > > > > > I would still like all ObjCXX tests for -Wthread-safety in one 
> > > > > > file, because that's what we have for the other languages as well. 
> > > > > > (more or less)
> > > > > > 
> > > > > > It's somewhat easier to see which aspects are tested then and which 
> > > > > > are not. Right now we only have tests for reported crashes, but 
> > > > > > maybe someday someone finds the time to test a bit more 
> > > > > > exhaustively.
> > > > > > 
> > > > > > But perhaps @aaron.ballman sees this another way, in that case 
> > > > > > follow his opinion.
> > > > > On one hand, the all-in-one test files get to be unwieldy size, but 
> > > > > on the other hand, it's nice having everything integrated into a 
> > > > > single file for testing purposes. I'd say go with the all-in-one 
> > > > > approach because it's consistent with how we otherwise test thread 
> > > > > safety and there are some (albeit, minor) benefits.
> > > > I'm not a fan of huge tests, and it's not consistent with other parts 
> > > > of LLVM. Specifically, this is testing for a regression, and should 
> > > > standalone to future edits don't refactor the test away. The existing 
> > > > test checks for functionality, and refactoring it is fine.
> > > I'm not going to insist, but the two people who do the most active work 
> > > in this area in recent history just told you what they prefer. ;-) We add 
> > > regression tests to the existing thread safety test files and it's worked 
> > > out just fine.
> > 4% of files under clang/test (and llvm/test) start with "PR".  I appreciate 
> > your being gentle... and I'll gently nudge folks working on thread safety 
> > to follow the established process clang and LLVM follow ;-)
> > Maybe I should file a bug to follow said process myself?
> > 
> > Basically, this file is here to prevent regressions. It really shouldn't 
> > change, ever, and I'd like to make this explicit.
> Thanks to an IRC discussion, I now understand the point @jfb is making about 
> this being a crashing regression test. You're right, we don't usually 
> incorporate those into larger functionality tests. It's less about the 
> regressions and more about the kind of regressions we're testing against.
> 
> I'm fine with the approach in this patch now.
The other test is not really a functional test, I added it in rC342600 as part 
of a fix for another bug that was reported as 
[PR38896](https://bugs.llvm.org/show_bug.cgi?id=38896). But the underlying 
cause for both bugs is not an oversight, it's that ObjC[++] is just not 
supported, and someone was too lazy to completely turn the analysis off or 
disallow it. Now people try it out and unsurprisingly it doesn't work. Your 
test is not some weird special case, so it could become part of a more 
systematic functional test.

In Clang it's quite usual to have one test per warning flag, and the thread 
safety analysis is just one flag. I guess the reason is that it makes 
refactoring easier: you see all the special cases in one file. That's quite 
nice! Have a look at some of these tests, they basically allow you to write the 
warning yourself. If all these special cases were spread around the code base, 
that would certainly make it harder to understand what a warning does.

The test for C++ also contains tests for at least two bugs: PR34800, PR38640. 
(I just grepped this, there is probably more. You can try `grep PR 
test/SemaCXX/warn-*`.) If you fear someone is going to change the test, rename 
your class from `MyInterface` to `PRX` or something else which indicates a 
crash.

Also indicate the issue it detects: that ObjC methods weren't handled in 
`SExprBuilder::translateDeclRefExpr`. Or that we test that they are handled 
now. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523



___
cfe-commits mailing list
cfe-commits@

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Creating a new test makes sense to me if it tests things across components. We 
have such tests for modules, PCH, and templates. There are also separate tests 
for the attribute parsing, which doesn't work terribly well in ObjC either. I 
would agree to making a new test for that when someone fixes PR38896.




Comment at: lib/Analysis/ThreadSafetyCommon.cpp:288
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }

Does your test run into this with an `ObjCMethodDecl`? I see how we run into 
the assignment to `VD` down below, but I don't understand how we could get here 
without annotating a method.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523



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

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

> It's less about the regressions and more about the kind of regressions we're 
> testing against.

But the test also verifies that no diagnostics are omitted (`// 
expected-no-diagnostics`), so it isn't just a "this doesn't crash" test. Which 
is why I think it's a nice seed to build more systematic tests around it. I'd 
generally like to test this more systematically, but that isn't made easier if 
we're sprinkling the test cases over the code base.

> Basically, this file is here to prevent regressions.

Isn't every test about exactly that? That there was a similar bug in the past 
is at best informative, but not necessarily relevant. And if a functional test 
crashes, isn't that just as bad? I don't understand why the historical reason 
for a test needs to have an influence on where the test is placed. It makes 
much more sense to me to group tests by the code they test.

If there is a unit test for a class and you find a bug in it that makes it 
crash, would you write a complete new unit test? Or would you add a test case 
to the existing test? These files are our “poor man's unit test” for warnings.




Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282
+if (Ctx && Ctx->FunArgs) {
+  const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+  if (isa(D)

jfb wrote:
> aaron.ballman wrote:
> > Same.
> It's a `Decl` (that's canonical), but same.
Depends on the type of `Ctx->AttrDecl`, some derived types of `Decl` have a 
stricter return type for `getCanonicalDecl()`. So I guess it boils down to what 
one thinks is 
[obvious](https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable).
 We're erring on the “be explicit” end of the scale here.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:288
+assert(I < Ctx->NumArgs);
+return translate(Ctx->FunArgs[I], Ctx->Prev);
+  }

jfb wrote:
> aaronpuchert wrote:
> > Does your test run into this with an `ObjCMethodDecl`? I see how we run 
> > into the assignment to `VD` down below, but I don't understand how we could 
> > get here without annotating a method.
> The problem is in the cast above:
> ```
> (lldb) 
> frame #5: clang::threadSafety::SExprBuilder::translateDeclRefExpr(this, DRE, 
> Ctx) at ThreadSafetyCommon.cpp:280:9
>277  // Function parameters require substitution and/or renaming.
>278  if (const auto *PV = dyn_cast_or_null(VD)) {
>279const auto *FD =
> -> 280
> cast(PV->getDeclContext())->getCanonicalDecl();
>281unsigned I = PV->getFunctionScopeIndex();
>282
>283if (Ctx && Ctx->FunArgs && FD == 
> Ctx->AttrDecl->getCanonicalDecl()) {
> ```
Ok. I just thought it would be interesting to see if can we run into the `if`, 
but I can try that myself.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523



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

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert accepted this revision.
aaronpuchert added a comment.

Alright, go ahead. I don't want this to be held up by such a minor detail.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523



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

[PATCH] D59523: Thread Safety: also look at ObjC methods

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D59523#1440263 , @aaron.ballman 
wrote:

> In D59523#1440238 , @jfb wrote:
>
> > This is very concrete: this specific code used to cause a crash. This test 
> > has exactly this purpose, nothing more. What I'm doing is extremely normal 
> > for LLVM.
>
>
> Agreed, this is a normal practice for tests verifying that a crash no longer 
> happens.


It certainly makes sense in many components, and I'm not questioning their 
judgement. But in the seven years prior to my involvement, no one thought it 
necessary to create a separate test for crashes in the thread safety analysis, 
and it's not because there haven't been any.

Warnings are in a different situations than the parser, optimizer passes or the 
backend, as they only operate read-only on the AST (or CFG). Looking at the 
commit log for `test/SemaCXX`, the majority of recent commits that fix crashes 
(and say so in the commit message) have been extending existing test cases 
instead of creating new ones. (Take for example rC337766 
, rC338089 
, rC342192 
, rC352047 
.) The thread safety analysis only looks at 
a single function's source-level CFG at a time. So we can reasonably consider 
functions as sufficiently isolated test cases and declarations as their setup.

That this doesn't work for a crash in the backend or parser is completely 
obvious to me. When working there, I would probably do the same and create a 
new file to reproduce my case in isolation. But what makes sense and is 
established practice in one component might not always make sense in another.

I'll leave the judgement to you, I'm just not convinced that this needs to be 
done.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59523



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D59673#1452269 , @dblaikie wrote:

> Ah, fair. You could actually test the dwo_name is accurate in the .dwo file 
> (I added the dwo_name to the .dwo file so that multi-level dwp error messages 
> could be more informative)


Ok, I'll just check the dwo_name for both files then.

>>> (& I guess there's an LLVM patch to add the rest of this functionality?)
>> 
>> What do we have to do there? Should we add the option to `llc` as well?
> 
> Yep - pretty much anything in MCOptions should be surfaced through llc for 
> llvm-level testing.

I was about to add this when I discovered that there is such an option already. 
The command line `clang -cc1 -split-dwarf-file a -fsplit-dwarf-dwo-name-attr b` 
corresponds to `llc -split-dwarf-output a -split-dwarf-file b`. That seems a 
bit unfortunate, especially the different meanings of `-split-dwarf-file`. 
Should/can we strive to emulate the behavior of llc in Clang instead? That 
would perhaps require changes to the behavior of `-split-dwarf-file`. Here's 
how the options behave with llc:

| Option  | ∅| `-split-dwarf-file a`
|
| ∅   | No split | Split DI in same object, set 
`DW_AT_GNU_dwo_name = a`, no extra file |
| `-split-dwarf-output b` | No split | Split DI, set `DW_AT_GNU_dwo_name = a`, 
separate file `b`|
|

Currently, Clang's `-enable-split-dwarf[=split] -split-dwarf-file a` does the 
same as `-split-dwarf-output a -split-dwarf-file a` in llc, and 
`-enable-split-dwarf=single -split-dwarf-file a` does the same as 
`-split-dwarf-file a` in llc.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Ok, here is an idea. We introduce `-split-dwarf-output` in Clang instead of 
`-fsplit-dwarf-dwo-name-attr`. If given, it overrides the output file name for 
the Split DWARF file, which we otherwise take from `-split-dwarf-file`. The 
option is obviously incompatible with `-enable-split-dwarf=single`, so we will 
disallow that. This should be backwards-compatible, and bring the behavior of 
`llc` and `clang -cc1` closer together. What do you think?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[Aaron Puchert via Phabricator via cfe-commits]

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

In D59673#1461983 , @dblaikie wrote:

> Sure, I think the naming's a bit weird (but hard to come up with good names 
> for any of this)


Agreed, `-split-dwarf-output` is pretty clear, but `-split-dwarf-file` could be 
both the actual filename or the attribute.

There is `test/CodeGen/split-debug-filename.c` checking that we emit 
`!DICompileUnit({{.*}}, splitDebugFilename: "foo.dwo"` into the IR under some 
circumstances, but I'm not sure why. It seems to be ignored by `llc`. What's 
the intended use for this? Your commit message in rC301063 
 suggests that this is needed for implicit 
modules. How does this work? I'm asking of course because I'm not sure whether 
we might need to emit both file names there.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[Aaron Puchert via Phabricator via cfe-commits]

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

Introduce -split-dwarf-output instead to mirror the behavior of llc, which 
already allows to set the attribute separately from the output file name.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673

Files:
  include/clang/Basic/CodeGenOptions.def
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/CC1Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-filename.c
  test/CodeGen/split-debug-output.c
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Misc/cc1as-split-dwarf.s
  tools/driver/cc1as_main.cpp

Index: tools/driver/cc1as_main.cpp
===
--- tools/driver/cc1as_main.cpp
+++ tools/driver/cc1as_main.cpp
@@ -98,7 +98,7 @@
   llvm::DebugCompressionType CompressDebugSections =
   llvm::DebugCompressionType::None;
   std::string MainFileName;
-  std::string SplitDwarfFile;
+  std::string SplitDwarfOutput;
 
   /// @}
   /// @name Frontend Options
@@ -258,7 +258,7 @@
   }
   Opts.LLVMArgs = Args.getAllArgValues(OPT_mllvm);
   Opts.OutputPath = Args.getLastArgValue(OPT_o);
-  Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
+  Opts.SplitDwarfOutput = Args.getLastArgValue(OPT_split_dwarf_output);
   if (Arg *A = Args.getLastArg(OPT_filetype)) {
 StringRef Name = A->getValue();
 unsigned OutputType = StringSwitch(Name)
@@ -367,8 +367,8 @@
   if (!FDOS)
 return true;
   std::unique_ptr DwoOS;
-  if (!Opts.SplitDwarfFile.empty())
-DwoOS = getOutputStream(Opts.SplitDwarfFile, Diags, IsBinary);
+  if (!Opts.SplitDwarfOutput.empty())
+DwoOS = getOutputStream(Opts.SplitDwarfOutput, Diags, IsBinary);
 
   // FIXME: This is not pretty. MCContext has a ptr to MCObjectFileInfo and
   // MCObjectFileInfo needs a MCContext reference in order to initialize itself.
@@ -527,8 +527,8 @@
   if (Failed) {
 if (Opts.OutputPath != "-")
   sys::fs::remove(Opts.OutputPath);
-if (!Opts.SplitDwarfFile.empty() && Opts.SplitDwarfFile != "-")
-  sys::fs::remove(Opts.SplitDwarfFile);
+if (!Opts.SplitDwarfOutput.empty() && Opts.SplitDwarfOutput != "-")
+  sys::fs::remove(Opts.SplitDwarfOutput);
   }
 
   return Failed;
Index: test/Misc/cc1as-split-dwarf.s
===
--- test/Misc/cc1as-split-dwarf.s
+++ test/Misc/cc1as-split-dwarf.s
@@ -1,5 +1,5 @@
 // REQUIRES: x86-registered-target
-// RUN: %clang -cc1as -triple x86_64-pc-linux-gnu %s -filetype obj -o %t1 -split-dwarf-file %t2
+// RUN: %clang -cc1as -triple x86_64-pc-linux-gnu %s -filetype obj -o %t1 -split-dwarf-output %t2
 // RUN: llvm-objdump -s %t1 | FileCheck --check-prefix=O %s
 // RUN: llvm-objdump -s %t2 | FileCheck --check-prefix=DWO %s
 
Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -3,7 +3,7 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
 //
-// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
+// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
@@ -13,13 +13,15 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS-SINGLE-SPLIT < %t %s
 //
-// CHECK-ACTIONS-SINGLE-SPLIT: "-enable-split-dwarf=single"
+// CHECK-ACTIONS-SINGLE-SPLIT: "-enable-split-dwarf"
 // CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-file" "split-debug.o"
+// CHECK-ACTIONS-SINGLE-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### -o %tfoo.o %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SINGLE-SPLIT-FILENAME < %t %s
 //
 // CHECK-SINGLE-SPLIT-FILENAME: "-split-dwarf-file" "{{.*}}foo.o"
+// CHECK-SINGLE-SPLIT-FILENAME-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
@@ -41,7 +43,7 @@
 // RUN: %clang -target amdgcn-amd-amdhsa -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-OPTION < %t %s
 //
-// CHECK-OPTION: "-split-dwarf-file" "split-debug.dwo"
+// CHECK-OPTION: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ASM < %t %s
@@ -59,6 +61,7 @@
 // CHECK-GMLT-WITH-SPLIT: "-enable-split-dwarf"
 // CHECK-GMLT-WITH-SPLIT: "-debug-inf

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a subscriber: pcc.
aaronpuchert added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:1345
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfFile;
+  Conf.DwoPath = CGOpts.SplitDwarfOutput;
   switch (Action) {

@pcc Your documentation for `DwoPath` suggests that this should be the actual 
output filename. However, the test that you added together with this line in 
rC333677 doesn't fail whatever garbage I write into that field here. What can I 
add to that so that it fails when we don't do the right thing here?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

[Aaron Puchert via Phabricator via cfe-commits]

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

Adapt one more test case.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673

Files:
  include/clang/Basic/CodeGenOptions.def
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/CC1Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-filename.c
  test/CodeGen/split-debug-output.c
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Driver/split-debug.s
  test/Misc/cc1as-split-dwarf.s
  tools/driver/cc1as_main.cpp

Index: tools/driver/cc1as_main.cpp
===
--- tools/driver/cc1as_main.cpp
+++ tools/driver/cc1as_main.cpp
@@ -98,7 +98,7 @@
   llvm::DebugCompressionType CompressDebugSections =
   llvm::DebugCompressionType::None;
   std::string MainFileName;
-  std::string SplitDwarfFile;
+  std::string SplitDwarfOutput;
 
   /// @}
   /// @name Frontend Options
@@ -258,7 +258,7 @@
   }
   Opts.LLVMArgs = Args.getAllArgValues(OPT_mllvm);
   Opts.OutputPath = Args.getLastArgValue(OPT_o);
-  Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
+  Opts.SplitDwarfOutput = Args.getLastArgValue(OPT_split_dwarf_output);
   if (Arg *A = Args.getLastArg(OPT_filetype)) {
 StringRef Name = A->getValue();
 unsigned OutputType = StringSwitch(Name)
@@ -367,8 +367,8 @@
   if (!FDOS)
 return true;
   std::unique_ptr DwoOS;
-  if (!Opts.SplitDwarfFile.empty())
-DwoOS = getOutputStream(Opts.SplitDwarfFile, Diags, IsBinary);
+  if (!Opts.SplitDwarfOutput.empty())
+DwoOS = getOutputStream(Opts.SplitDwarfOutput, Diags, IsBinary);
 
   // FIXME: This is not pretty. MCContext has a ptr to MCObjectFileInfo and
   // MCObjectFileInfo needs a MCContext reference in order to initialize itself.
@@ -527,8 +527,8 @@
   if (Failed) {
 if (Opts.OutputPath != "-")
   sys::fs::remove(Opts.OutputPath);
-if (!Opts.SplitDwarfFile.empty() && Opts.SplitDwarfFile != "-")
-  sys::fs::remove(Opts.SplitDwarfFile);
+if (!Opts.SplitDwarfOutput.empty() && Opts.SplitDwarfOutput != "-")
+  sys::fs::remove(Opts.SplitDwarfOutput);
   }
 
   return Failed;
Index: test/Misc/cc1as-split-dwarf.s
===
--- test/Misc/cc1as-split-dwarf.s
+++ test/Misc/cc1as-split-dwarf.s
@@ -1,5 +1,5 @@
 // REQUIRES: x86-registered-target
-// RUN: %clang -cc1as -triple x86_64-pc-linux-gnu %s -filetype obj -o %t1 -split-dwarf-file %t2
+// RUN: %clang -cc1as -triple x86_64-pc-linux-gnu %s -filetype obj -o %t1 -split-dwarf-output %t2
 // RUN: llvm-objdump -s %t1 | FileCheck --check-prefix=O %s
 // RUN: llvm-objdump -s %t2 | FileCheck --check-prefix=DWO %s
 
Index: test/Driver/split-debug.s
===
--- test/Driver/split-debug.s
+++ test/Driver/split-debug.s
@@ -3,7 +3,7 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
 //
-// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
+// CHECK-ACTIONS: "-split-dwarf-output" "split-debug.dwo"
 
 // Check we pass -split-dwarf-file to `as` if -gsplit-dwarf=split.
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=split -c -### %s 2> %t
Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -3,7 +3,7 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
 //
-// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
+// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
@@ -13,13 +13,15 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS-SINGLE-SPLIT < %t %s
 //
-// CHECK-ACTIONS-SINGLE-SPLIT: "-enable-split-dwarf=single"
+// CHECK-ACTIONS-SINGLE-SPLIT: "-enable-split-dwarf"
 // CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-file" "split-debug.o"
+// CHECK-ACTIONS-SINGLE-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### -o %tfoo.o %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SINGLE-SPLIT-FILENAME < %t %s
 //
 // CHECK-SINGLE-SPLIT-FILENAME: "-split-dwarf-file" "{{.*}}foo.o"
+// CHECK-SINGLE-SPLIT-FILENAME-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
@@ -41,7 +43,7 @@
 // RUN: %clang -target amdgc

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert edited reviewers, added: riccibruno; removed: efriedma.
aaronpuchert removed a subscriber: riccibruno.
aaronpuchert added a comment.

Ping?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59402



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

[PATCH] D52141: Thread safety analysis: Run more tests with capability attributes [NFC]

[Aaron Puchert via Phabricator via cfe-commits]

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

We run the tests for -Wthread-safety-{negative,verbose} with the new
attributes as well as the old ones. Also put the macros in a header so
that we don't have to copy them all around.

The warn-thread-safety-parsing.cpp test checks for warnings depending on
the actual attribute name, so it can't undergo the same treatment.

Together with https://reviews.llvm.org/D49275 this should fix PR33754.


Repository:
  rC Clang

https://reviews.llvm.org/D52141

Files:
  test/SemaCXX/thread-safety-annotations.h
  test/SemaCXX/warn-thread-safety-analysis.cpp
  test/SemaCXX/warn-thread-safety-negative.cpp
  test/SemaCXX/warn-thread-safety-verbose.cpp

Index: test/SemaCXX/warn-thread-safety-verbose.cpp
===
--- test/SemaCXX/warn-thread-safety-verbose.cpp
+++ test/SemaCXX/warn-thread-safety-verbose.cpp
@@ -1,37 +1,15 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-verbose -Wno-thread-safety-negative -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-verbose -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-verbose -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
 
-#define LOCKABLE__attribute__ ((lockable))
-#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable))
-#define GUARDED_BY(x)   __attribute__ ((guarded_by(x)))
-#define GUARDED_VAR __attribute__ ((guarded_var))
-#define PT_GUARDED_BY(x)__attribute__ ((pt_guarded_by(x)))
-#define PT_GUARDED_VAR  __attribute__ ((pt_guarded_var))
-#define ACQUIRED_AFTER(...) __attribute__ ((acquired_after(__VA_ARGS__)))
-#define ACQUIRED_BEFORE(...) __attribute__ ((acquired_before(__VA_ARGS__)))
-#define EXCLUSIVE_LOCK_FUNCTION(...)__attribute__ ((exclusive_lock_function(__VA_ARGS__)))
-#define SHARED_LOCK_FUNCTION(...)   __attribute__ ((shared_lock_function(__VA_ARGS__)))
-#define ASSERT_EXCLUSIVE_LOCK(...)  __attribute__ ((assert_exclusive_lock(__VA_ARGS__)))
-#define ASSERT_SHARED_LOCK(...) __attribute__ ((assert_shared_lock(__VA_ARGS__)))
-#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__ ((exclusive_trylock_function(__VA_ARGS__)))
-#define SHARED_TRYLOCK_FUNCTION(...)__attribute__ ((shared_trylock_function(__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__)))
-#define EXCLUSIVE_LOCKS_REQUIRED(...) \
-  __attribute__ ((exclusive_locks_required(__VA_ARGS__)))
-#define SHARED_LOCKS_REQUIRED(...) \
-  __attribute__ ((shared_locks_required(__VA_ARGS__)))
-#define NO_THREAD_SAFETY_ANALYSIS  __attribute__ ((no_thread_safety_analysis))
+#include "thread-safety-annotations.h"
 
-
-class  __attribute__((lockable)) Mutex {
+class LOCKABLE Mutex {
  public:
-  void Lock() __attribute__((exclusive_lock_function));
-  void ReaderLock() __attribute__((shared_lock_function));
-  void Unlock() __attribute__((unlock_function));
-  bool TryLock() __attribute__((exclusive_trylock_function(true)));
-  bool ReaderTryLock() __attribute__((shared_trylock_function(true)));
-  void LockWhen(const int &cond) __attribute__((exclusive_lock_function));
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+  bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true);
+  bool ReaderTryLock() SHARED_TRYLOCK_FUNCTION(true);
 
   // for negative capabilities
   const Mutex& operator!() const { return *this; }
Index: test/SemaCXX/warn-thread-safety-negative.cpp
===
--- test/SemaCXX/warn-thread-safety-negative.cpp
+++ test/SemaCXX/warn-thread-safety-negative.cpp
@@ -1,40 +1,18 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-negative -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
 
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
 
-#define LOCKABLE__attribute__ ((lockable))
-#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable))
-#define GUARDED_BY(x)   __attribute__ ((guarded_

[PATCH] D52141: Thread safety analysis: Run more tests with capability attributes [NFC]

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked an inline comment as done.
aaronpuchert added a comment.

Makes sense to me. Thanks for the review!


Repository:
  rC Clang

https://reviews.llvm.org/D52141



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

[PATCH] D52141: Thread safety analysis: Run more tests with capability attributes [NFC]

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC342418: Thread safety analysis: Run more tests with 
capability attributes [NFC] (authored by aaronpuchert, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52141?vs=165662&id=165835#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52141

Files:
  test/SemaCXX/thread-safety-annotations.h
  test/SemaCXX/warn-thread-safety-analysis.cpp
  test/SemaCXX/warn-thread-safety-negative.cpp
  test/SemaCXX/warn-thread-safety-verbose.cpp

Index: test/SemaCXX/thread-safety-annotations.h
===
--- test/SemaCXX/thread-safety-annotations.h
+++ test/SemaCXX/thread-safety-annotations.h
@@ -0,0 +1,42 @@
+// Macros to enable testing of both variants of the thread safety analysis.
+
+#if USE_CAPABILITY
+#define LOCKABLE__attribute__((capability("mutex")))
+#define ASSERT_EXCLUSIVE_LOCK(...)  __attribute__((assert_capability(__VA_ARGS__)))
+#define ASSERT_SHARED_LOCK(...) __attribute__((assert_shared_capability(__VA_ARGS__)))
+#define EXCLUSIVE_LOCK_FUNCTION(...)__attribute__((acquire_capability(__VA_ARGS__)))
+#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_LOCKS_REQUIRED(...)   __attribute__((requires_capability(__VA_ARGS__)))
+#define SHARED_LOCKS_REQUIRED(...)  __attribute__((requires_shared_capability(__VA_ARGS__)))
+#else
+#define LOCKABLE__attribute__((lockable))
+#define ASSERT_EXCLUSIVE_LOCK(...)  __attribute__((assert_exclusive_lock(__VA_ARGS__)))
+#define ASSERT_SHARED_LOCK(...) __attribute__((assert_shared_lock(__VA_ARGS__)))
+#define EXCLUSIVE_LOCK_FUNCTION(...)__attribute__((exclusive_lock_function(__VA_ARGS__)))
+#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_LOCKS_REQUIRED(...)   __attribute__((exclusive_locks_required(__VA_ARGS__)))
+#define SHARED_LOCKS_REQUIRED(...)  __attribute__((shared_locks_required(__VA_ARGS__)))
+#endif
+
+// Lock semantics only
+#define UNLOCK_FUNCTION(...)__attribute__((unlock_function(__VA_ARGS__)))
+#define GUARDED_VAR __attribute__((guarded_var))
+#define PT_GUARDED_VAR  __attribute__((pt_guarded_var))
+
+// Capabilities only
+#define EXCLUSIVE_UNLOCK_FUNCTION(...)  __attribute__((release_capability(__VA_ARGS__)))
+#define SHARED_UNLOCK_FUNCTION(...) __attribute__((release_shared_capability(__VA_ARGS__)))
+#define GUARDED_BY(x)   __attribute__((guarded_by(x)))
+#define PT_GUARDED_BY(x)__attribute__((pt_guarded_by(x)))
+
+// Common
+#define SCOPED_LOCKABLE __attribute__((scoped_lockable))
+#define ACQUIRED_AFTER(...) __attribute__((acquired_after(__VA_ARGS__)))
+#define ACQUIRED_BEFORE(...)__attribute__((acquired_before(__VA_ARGS__)))
+#define LOCK_RETURNED(x)__attribute__((lock_returned(x)))
+#define LOCKS_EXCLUDED(...) __attribute__((locks_excluded(__VA_ARGS__)))
+#define NO_THREAD_SAFETY_ANALYSIS   __attribute__((no_thread_safety_analysis))
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -6,42 +6,7 @@
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
 
-#define SCOPED_LOCKABLE  __attribute__((scoped_lockable))
-#define GUARDED_BY(x)__attribute__((guarded_by(x)))
-#define GUARDED_VAR  __attribute__((guarded_var))
-#define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x)))
-#define PT_GUARDED_VAR   __attribute__((pt_guarded_var))
-#define ACQUIRED_AFTER(...)  __attribute__((acquired_after(__VA_ARGS__)))
-#define ACQUIRED_BEFORE(...) __attribute__((acquired_before(__VA_ARGS__)))
-
-#if USE_CAPABILITY
-#define LOCKABLE__attribute__((capability("mutex")))
-#define ASSERT_EXCLUSIVE_LOCK(...)  __attribute__((assert_capability(__VA_ARGS__)))
-#define ASSERT_SHARED_LOCK(...) __attribute__((assert_shared_capability(__VA_ARGS__)))
-#define EXCLUSIVE_LOCK_FUNCTION(...)__attribute__((acquire_capability(__VA_ARGS__)))
-#define SHARED_LOCK_FUNCTION(...)   __attribute__((acquire_shared_capability(

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

[Aaron Puchert via Phabricator via cfe-commits]

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

This imitates the code for MemberExpr. I hope it is right, for I have
absolutely no understanding of ObjC++.

Fixes 38896.


Repository:
  rC Clang

https://reviews.llvm.org/D52200

Files:
  include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  lib/Analysis/ThreadSafetyCommon.cpp
  test/SemaObjCXX/warn-thread-safety-analysis.mm


Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta 
-Wno-objc-root-class %s
+
+class __attribute__((lockable)) Lock {
+public:
+  void Acquire() __attribute__((exclusive_lock_function())) {}
+  void Release() __attribute__((unlock_function())) {}
+};
+
+class __attribute__((scoped_lockable)) AutoLock {
+public:
+  AutoLock(Lock &lock) __attribute__((exclusive_lock_function(lock)))
+  : lock_(lock) {
+lock.Acquire();
+  }
+  ~AutoLock() __attribute__((unlock_function())) { lock_.Release(); }
+
+private:
+  Lock &lock_;
+};
+
+@interface MyInterface {
+@private
+  Lock lock_;
+  int value_;
+}
+
+- (void)incrementValue;
+- (void)decrementValue;
+
+@end
+
+@implementation MyInterface
+
+- (void)incrementValue {
+  AutoLock lock(lock_);
+  value_ += 1;
+}
+
+- (void)decrementValue {
+  lock_.Acquire(); // expected-note{{mutex acquired here}}
+  value_ -= 1;
+} // expected-warning{{mutex 'self.lock_' is still held at the end of 
function}}
+
+@end
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -211,6 +211,8 @@
 return translateCXXThisExpr(cast(S), Ctx);
   case Stmt::MemberExprClass:
 return translateMemberExpr(cast(S), Ctx);
+  case Stmt::ObjCIvarRefExprClass:
+return translateObjCIVarRefExpr(cast(S), Ctx);
   case Stmt::CallExprClass:
 return translateCallExpr(cast(S), Ctx);
   case Stmt::CXXMemberCallExprClass:
@@ -349,6 +351,19 @@
   return P;
 }
 
+til::SExpr *SExprBuilder::translateObjCIVarRefExpr(const ObjCIvarRefExpr *IVRE,
+   CallingContext *Ctx) {
+  til::SExpr *BE = translate(IVRE->getBase(), Ctx);
+  til::SExpr *E = new (Arena) til::SApply(BE);
+
+  const auto *D = cast(IVRE->getDecl()->getCanonicalDecl());
+
+  til::Project *P = new (Arena) til::Project(E, D);
+  if (hasCppPointerType(BE))
+P->setArrow(true);
+  return P;
+}
+
 til::SExpr *SExprBuilder::translateCallExpr(const CallExpr *CE,
 CallingContext *Ctx,
 const Expr *SelfE) {
Index: include/clang/Analysis/Analyses/ThreadSafetyCommon.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -397,6 +397,8 @@
CallingContext *Ctx) ;
   til::SExpr *translateCXXThisExpr(const CXXThisExpr *TE, CallingContext *Ctx);
   til::SExpr *translateMemberExpr(const MemberExpr *ME, CallingContext *Ctx);
+  til::SExpr *translateObjCIVarRefExpr(const ObjCIvarRefExpr *ME,
+   CallingContext *Ctx);
   til::SExpr *translateCallExpr(const CallExpr *CE, CallingContext *Ctx,
 const Expr *SelfE = nullptr);
   til::SExpr *translateCXXMemberCallExpr(const CXXMemberCallExpr *ME,


Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -Wno-objc-root-class %s
+
+class __attribute__((lockable)) Lock {
+public:
+  void Acquire() __attribute__((exclusive_lock_function())) {}
+  void Release() __attribute__((unlock_function())) {}
+};
+
+class __attribute__((scoped_lockable)) AutoLock {
+public:
+  AutoLock(Lock &lock) __attribute__((exclusive_lock_function(lock)))
+  : lock_(lock) {
+lock.Acquire();
+  }
+  ~AutoLock() __attribute__((unlock_function())) { lock_.Release(); }
+
+private:
+  Lock &lock_;
+};
+
+@interface MyInterface {
+@private
+  Lock lock_;
+  int value_;
+}
+
+- (void)incrementValue;
+- (void)decrementValue;
+
+@end
+
+@implementation MyInterface
+
+- (void)incrementValue {
+  AutoLock lock(lock_);
+  value_ += 1;
+}
+
+- (void)decrementValue {
+  lock_.Acquire(); // expected-note{{mutex acquired here}}
+  value_ -= 1;
+} // expected-warning{{mutex 'self.lock_' is still held at the end of function}}
+
+@end
Index: lib/Analysis/ThreadSafetyCommon.cpp
=

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

[Aaron Puchert via Phabricator via cfe-commits]

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

Thanks to both of you for the reviews. I'll see what I can do about the arrows. 
My gut feeling is that using `{Member,ObjCIVarRef}Expr::isArrow` is the right 
way, but it's not yet obvious to me how.




Comment at: include/clang/Analysis/Analyses/ThreadSafetyCommon.h:400
   til::SExpr *translateMemberExpr(const MemberExpr *ME, CallingContext *Ctx);
+  til::SExpr *translateObjCIVarRefExpr(const ObjCIvarRefExpr *ME,
+   CallingContext *Ctx);

aaron.ballman wrote:
> `ME` is kind of a poor name; can you switch to `IVRE` like you used in the 
> implementation?
Of course, that's a copy-and-paste error.



Comment at: lib/Analysis/ThreadSafetyCommon.cpp:362
+  til::Project *P = new (Arena) til::Project(E, D);
+  if (hasCppPointerType(BE))
+P->setArrow(true);

rjmccall wrote:
> aaron.ballman wrote:
> > I feel like this will always return false. However, I wonder if 
> > `IVRE->getBase()->isArrow()` is more appropriate here?
> The base of an `ObjCIvarRefExpr` will never directly be a C pointer type.  I 
> don't know whether this data structure looks through casts, but it's 
> certainly *possible* to cast arbitrarily weird C stuff to ObjC pointer type.  
> On the other hand, by and large nobody actually ever does that, so I wouldn't 
> make a special case for it here.
> 
> `ObjCIvarRefExpr` just has an `isArrow()` method directly if this is just 
> supposed to be capturing the difference between `.` and `->`.  But then, so 
> does `MemberExpr`, and in that case you're doing this odd analysis of the 
> base instead of trusting `isArrow()`, so I don't know what to tell you to do.
> 
> Overall it is appropriate to treat an ObjC ivar reference as a perfectly 
> ordinary projection.  The only thing that's special about it is that the 
> actual offset isn't necessarily statically known, but ivars still behave as 
> if they're all independently declared, so I wouldn't worry about it.
I had wondered about this as well, but when I changed `hasCppPointerType(BE)` 
to `ME->isArrow()` in the `MemberExpr` case, I got some test failures. For 
example:

```
struct Foo {
  int a __attribute__((guarded_by(mu)));
  Mutex mu;
};

void f() {
  Foo foo;
  foo.a = 5; // \
// expected-warning{{writing variable 'a' requires holding mutex 'foo.mu' 
exclusively}}
}
```

Instead we warn `writing variable 'a' requires holding mutex 'foo->mu' 
exclusively`. That's not right.

The expression (`ME`) we are processing is `mu` from the attribute on `a`, 
which the compiler sees as `this->mu`. So we get `ME->isArrow() == true` and 
`ME->getBase()` is a `CXXThisExpr`.

Basically the `translate*` functions do a substitution. They try to derive 
`foo.mu` from `this->mu` on the `a` member and the expression `foo.a`. The 
annotation `this->mu` is the expression we get as parameter, and `foo.a` is 
encoded in the `CallingContext`.

The information whether `foo.a` is an arrow (it isn't) seems to be in the 
`CallingContext`'s `SelfArrow` member.


Repository:
  rC Clang

https://reviews.llvm.org/D52200



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

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

I found something that would theoretically work:

  P->setArrow((isa(ME->getBase()) && Ctx && Ctx->SelfArg) ? 
Ctx->SelfArrow : ME->isArrow());

So if we have `this` and a context that tells us we have to replace `this` by 
something else, then we check `Ctx->SelfArrow`, otherwise we take 
`ME->isArrow()`.

But that doesn't work. When translating into the TIL (typed intermediate 
language), referencing and dereferencing operators are completely ignored.

  struct Foo {
Mutex mu_;
void foo1(Foo *f_declared) EXCLUSIVE_LOCKS_REQUIRED(f_declared->mu_);
  };
  
  void test() {
Foo myfoo;
myfoo.foo1(&myfoo);  // \
  // expected-warning {{calling function 'foo1' requires holding mutex 
'myfoo.mu_' exclusively}}
  }

With the above change we warn that `calling function 'foo1' requires holding 
mutex 'myfoo->mu_' exclusively`. It should be `(&myfoo)->mu_`, but the `&` is 
lost. So we can't derive the information that we want from `isArrow` alone.

Now there is a reason why these operators are ignored — the TIL tries to 
"canonicalize" expressions, so that it detects that `(&myfoo)->mu_` and 
`myfoo.mu_` are the same thing. Changing that is probably possible, but beyond 
the scope of this change.

Short of that, we must be able to detect pointers. I think we could use 
`Type::isAnyPointerType` instead of `Type::isPointerType` in 
`hasCppPointerType` (and probably rename that).

For later I think we should consider a different canonicalization that doesn't 
just ignore `&` and `*`.


Repository:
  rC Clang

https://reviews.llvm.org/D52200



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

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

[Aaron Puchert via Phabricator via cfe-commits]

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

Detect ObjC pointer types as well as ordinary pointers.


Repository:
  rC Clang

https://reviews.llvm.org/D52200

Files:
  include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  lib/Analysis/ThreadSafetyCommon.cpp
  test/SemaObjCXX/warn-thread-safety-analysis.mm

Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -Wno-objc-root-class %s
+
+class __attribute__((lockable)) Lock {
+public:
+  void Acquire() __attribute__((exclusive_lock_function())) {}
+  void Release() __attribute__((unlock_function())) {}
+};
+
+class __attribute__((scoped_lockable)) AutoLock {
+public:
+  AutoLock(Lock &lock) __attribute__((exclusive_lock_function(lock)))
+  : lock_(lock) {
+lock.Acquire();
+  }
+  ~AutoLock() __attribute__((unlock_function())) { lock_.Release(); }
+
+private:
+  Lock &lock_;
+};
+
+@interface MyInterface {
+@private
+  Lock lock_;
+  int value_;
+}
+
+- (void)incrementValue;
+- (void)decrementValue;
+
+@end
+
+@implementation MyInterface
+
+- (void)incrementValue {
+  AutoLock lock(lock_);
+  value_ += 1;
+}
+
+- (void)decrementValue {
+  lock_.Acquire(); // expected-note{{mutex acquired here}}
+  value_ -= 1;
+} // expected-warning{{mutex 'self->lock_' is still held at the end of function}}
+
+@end
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -211,6 +211,8 @@
 return translateCXXThisExpr(cast(S), Ctx);
   case Stmt::MemberExprClass:
 return translateMemberExpr(cast(S), Ctx);
+  case Stmt::ObjCIvarRefExprClass:
+return translateObjCIVarRefExpr(cast(S), Ctx);
   case Stmt::CallExprClass:
 return translateCallExpr(cast(S), Ctx);
   case Stmt::CXXMemberCallExprClass:
@@ -311,9 +313,9 @@
   return nullptr;
 }
 
-static bool hasCppPointerType(const til::SExpr *E) {
+static bool hasAnyPointerType(const til::SExpr *E) {
   auto *VD = getValueDeclFromSExpr(E);
-  if (VD && VD->getType()->isPointerType())
+  if (VD && VD->getType()->isAnyPointerType())
 return true;
   if (const auto *C = dyn_cast(E))
 return C->castOpcode() == til::CAST_objToPtr;
@@ -344,7 +346,20 @@
 D = getFirstVirtualDecl(VD);
 
   til::Project *P = new (Arena) til::Project(E, D);
-  if (hasCppPointerType(BE))
+  if (hasAnyPointerType(BE))
+P->setArrow(true);
+  return P;
+}
+
+til::SExpr *SExprBuilder::translateObjCIVarRefExpr(const ObjCIvarRefExpr *IVRE,
+   CallingContext *Ctx) {
+  til::SExpr *BE = translate(IVRE->getBase(), Ctx);
+  til::SExpr *E = new (Arena) til::SApply(BE);
+
+  const auto *D = cast(IVRE->getDecl()->getCanonicalDecl());
+
+  til::Project *P = new (Arena) til::Project(E, D);
+  if (hasAnyPointerType(BE))
 P->setArrow(true);
   return P;
 }
Index: include/clang/Analysis/Analyses/ThreadSafetyCommon.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -397,6 +397,8 @@
CallingContext *Ctx) ;
   til::SExpr *translateCXXThisExpr(const CXXThisExpr *TE, CallingContext *Ctx);
   til::SExpr *translateMemberExpr(const MemberExpr *ME, CallingContext *Ctx);
+  til::SExpr *translateObjCIVarRefExpr(const ObjCIvarRefExpr *IVRE,
+   CallingContext *Ctx);
   til::SExpr *translateCallExpr(const CallExpr *CE, CallingContext *Ctx,
 const Expr *SelfE = nullptr);
   til::SExpr *translateCXXMemberCallExpr(const CXXMemberCallExpr *ME,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked 2 inline comments as done.
aaronpuchert added a comment.

I think it should be possible to get rid of `self->` in the warning message if 
we want to, after all `this->` is omitted in C++ as well.




Comment at: test/SemaObjCXX/warn-thread-safety-analysis.mm:42
+  value_ -= 1;
+} // expected-warning{{mutex 'self->lock_' is still held at the end of 
function}}
+

@rjmccall Would you rather write `self->lock_` or `lock_` in the warning 
message?


Repository:
  rC Clang

https://reviews.llvm.org/D52200



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

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

It seems that `self` is an ordinary `DeclRefExpr` unlike `this`, which is a 
`CXXThisExpr`. Which means we'd have to make it dependent on the name whether 
we drop it, but `self` in C/C++ is just an ordinary variable. So I think I'll 
leave the `self->` part for now. It certainly doesn't hurt.


Repository:
  rC Clang

https://reviews.llvm.org/D52200



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

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC342600: Thread safety analysis: Handle ObjCIvarRefExpr in 
SExprBuilder::translate (authored by aaronpuchert, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52200?vs=166051&id=166197#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52200

Files:
  include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  lib/Analysis/ThreadSafetyCommon.cpp
  test/SemaObjCXX/warn-thread-safety-analysis.mm

Index: include/clang/Analysis/Analyses/ThreadSafetyCommon.h
===
--- include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -397,6 +397,8 @@
CallingContext *Ctx) ;
   til::SExpr *translateCXXThisExpr(const CXXThisExpr *TE, CallingContext *Ctx);
   til::SExpr *translateMemberExpr(const MemberExpr *ME, CallingContext *Ctx);
+  til::SExpr *translateObjCIVarRefExpr(const ObjCIvarRefExpr *IVRE,
+   CallingContext *Ctx);
   til::SExpr *translateCallExpr(const CallExpr *CE, CallingContext *Ctx,
 const Expr *SelfE = nullptr);
   til::SExpr *translateCXXMemberCallExpr(const CXXMemberCallExpr *ME,
Index: test/SemaObjCXX/warn-thread-safety-analysis.mm
===
--- test/SemaObjCXX/warn-thread-safety-analysis.mm
+++ test/SemaObjCXX/warn-thread-safety-analysis.mm
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -Wno-objc-root-class %s
+
+class __attribute__((lockable)) Lock {
+public:
+  void Acquire() __attribute__((exclusive_lock_function())) {}
+  void Release() __attribute__((unlock_function())) {}
+};
+
+class __attribute__((scoped_lockable)) AutoLock {
+public:
+  AutoLock(Lock &lock) __attribute__((exclusive_lock_function(lock)))
+  : lock_(lock) {
+lock.Acquire();
+  }
+  ~AutoLock() __attribute__((unlock_function())) { lock_.Release(); }
+
+private:
+  Lock &lock_;
+};
+
+@interface MyInterface {
+@private
+  Lock lock_;
+  int value_;
+}
+
+- (void)incrementValue;
+- (void)decrementValue;
+
+@end
+
+@implementation MyInterface
+
+- (void)incrementValue {
+  AutoLock lock(lock_);
+  value_ += 1;
+}
+
+- (void)decrementValue {
+  lock_.Acquire(); // expected-note{{mutex acquired here}}
+  value_ -= 1;
+} // expected-warning{{mutex 'self->lock_' is still held at the end of function}}
+
+@end
Index: lib/Analysis/ThreadSafetyCommon.cpp
===
--- lib/Analysis/ThreadSafetyCommon.cpp
+++ lib/Analysis/ThreadSafetyCommon.cpp
@@ -211,6 +211,8 @@
 return translateCXXThisExpr(cast(S), Ctx);
   case Stmt::MemberExprClass:
 return translateMemberExpr(cast(S), Ctx);
+  case Stmt::ObjCIvarRefExprClass:
+return translateObjCIVarRefExpr(cast(S), Ctx);
   case Stmt::CallExprClass:
 return translateCallExpr(cast(S), Ctx);
   case Stmt::CXXMemberCallExprClass:
@@ -311,9 +313,9 @@
   return nullptr;
 }
 
-static bool hasCppPointerType(const til::SExpr *E) {
+static bool hasAnyPointerType(const til::SExpr *E) {
   auto *VD = getValueDeclFromSExpr(E);
-  if (VD && VD->getType()->isPointerType())
+  if (VD && VD->getType()->isAnyPointerType())
 return true;
   if (const auto *C = dyn_cast(E))
 return C->castOpcode() == til::CAST_objToPtr;
@@ -344,7 +346,20 @@
 D = getFirstVirtualDecl(VD);
 
   til::Project *P = new (Arena) til::Project(E, D);
-  if (hasCppPointerType(BE))
+  if (hasAnyPointerType(BE))
+P->setArrow(true);
+  return P;
+}
+
+til::SExpr *SExprBuilder::translateObjCIVarRefExpr(const ObjCIvarRefExpr *IVRE,
+   CallingContext *Ctx) {
+  til::SExpr *BE = translate(IVRE->getBase(), Ctx);
+  til::SExpr *E = new (Arena) til::SApply(BE);
+
+  const auto *D = cast(IVRE->getDecl()->getCanonicalDecl());
+
+  til::Project *P = new (Arena) til::Project(E, D);
+  if (hasAnyPointerType(BE))
 P->setArrow(true);
   return P;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked an inline comment as done.
aaronpuchert added a comment.

In https://reviews.llvm.org/D51901#1239759, @delesley wrote:

> This looks okay to me, but I have not tested it on a large code base to see 
> if it breaks anything.


On our code base (not as large as Google's) there was one true positive and no 
false positives. At least for templates we should be fine now, since we assume 
that all type-dependent base classes could have any attribute.


Repository:
  rC Clang

https://reviews.llvm.org/D51901



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

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC342605: Thread Safety Analysis: warnings for attributes 
without arguments (authored by aaronpuchert, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D51901?vs=165004&id=166205#toc

Repository:
  rC Clang

https://reviews.llvm.org/D51901

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-capabilities.c
  test/SemaCXX/warn-thread-safety-parsing.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -476,6 +476,29 @@
   return nullptr;
 }
 
+template 
+static bool checkRecordDeclForAttr(const RecordDecl *RD) {
+  // Check if the record itself has the attribute.
+  if (RD->hasAttr())
+return true;
+
+  // Else check if any base classes have the attribute.
+  if (const auto *CRD = dyn_cast(RD)) {
+CXXBasePaths BPaths(false, false);
+if (CRD->lookupInBases(
+[](const CXXBaseSpecifier *BS, CXXBasePath &) {
+  const auto &Ty = *BS->getType();
+  // If it's type-dependent, we assume it could have the attribute.
+  if (Ty.isDependentType())
+return true;
+  return Ty.getAs()->getDecl()->hasAttr();
+},
+BPaths, true))
+  return true;
+  }
+  return false;
+}
+
 static bool checkRecordTypeForCapability(Sema &S, QualType Ty) {
   const RecordType *RT = getRecordType(Ty);
 
@@ -491,21 +514,7 @@
   if (threadSafetyCheckIsSmartPointer(S, RT))
 return true;
 
-  // Check if the record itself has a capability.
-  RecordDecl *RD = RT->getDecl();
-  if (RD->hasAttr())
-return true;
-
-  // Else check if any base classes have a capability.
-  if (const auto *CRD = dyn_cast(RD)) {
-CXXBasePaths BPaths(false, false);
-if (CRD->lookupInBases([](const CXXBaseSpecifier *BS, CXXBasePath &) {
-  const auto *Type = BS->getType()->getAs();
-  return Type->getDecl()->hasAttr();
-}, BPaths))
-  return true;
-  }
-  return false;
+  return checkRecordDeclForAttr(RT->getDecl());
 }
 
 static bool checkTypedefTypeForCapability(QualType Ty) {
@@ -563,8 +572,27 @@
 static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
const ParsedAttr &AL,
SmallVectorImpl &Args,
-   int Sidx = 0,
+   unsigned Sidx = 0,
bool ParamIdxOk = false) {
+  if (Sidx == AL.getNumArgs()) {
+// If we don't have any capability arguments, the attribute implicitly
+// refers to 'this'. So we need to make sure that 'this' exists, i.e. we're
+// a non-static method, and that the class is a (scoped) capability.
+const auto *MD = dyn_cast(D);
+if (MD && !MD->isStatic()) {
+  const CXXRecordDecl *RD = MD->getParent();
+  // FIXME -- need to check this again on template instantiation
+  if (!checkRecordDeclForAttr(RD) &&
+  !checkRecordDeclForAttr(RD))
+S.Diag(AL.getLoc(),
+   diag::warn_thread_attribute_not_on_capability_member)
+<< AL << MD->getParent();
+} else {
+  S.Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_non_static_member)
+  << AL;
+}
+  }
+
   for (unsigned Idx = Sidx; Idx < AL.getNumArgs(); ++Idx) {
 Expr *ArgExp = AL.getArgAsExpr(Idx);
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -3008,6 +3008,14 @@
 def warn_thread_attribute_ignored : Warning<
   "ignoring %0 attribute because its argument is invalid">,
   InGroup, DefaultIgnore;
+def warn_thread_attribute_not_on_non_static_member : Warning<
+  "%0 attribute without capability arguments can only be applied to non-static "
+  "methods of a class">,
+  InGroup, DefaultIgnore;
+def warn_thread_attribute_not_on_capability_member : Warning<
+  "%0 attribute without capability arguments refers to 'this', but %1 isn't "
+  "annotated with 'capability' or 'scoped_lockable' attribute">,
+  InGroup, DefaultIgnore;
 def warn_thread_attribute_argument_not_lockable : Warning<
   "%0 attribute requires arguments whose type is annotated "
   "with 'capability' attribute; type here is %1">,
Index: test/SemaCXX/warn-thread-safety-parsing.cpp
===
--- test/SemaCXX/warn-thread-safety-parsing.cpp
+++ test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -572,11 +572,11 @@
 
 // takes zero or more arguments, all locks (vars/fields)
 
-void elf_function() EXCLUSIVE_LOCK_FUNCTION();
+void elf_function() EXCLUSIVE_LOCK_FUNCTION(); // expected-warning {{'exclusive_lock_f

[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Thanks for pointing out my error! Ignoring the implementation for a moment, do 
you think this is a good idea or will it produce to many false positives? 
Releasable/relockable scoped capabilities that I have seen keep track of the 
status, so it makes sense, but maybe there are others out there.




Comment at: lib/Analysis/ThreadSafety.cpp:929
+  return Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc());
+Locked = true;
+

delesley wrote:
> It's been a while since I last looked at this code, but I don't think you can 
> use mutable fields in a FactEntry.  The analysis creates a FactSet for each 
> program point, but each FactSet simply has pointers (FactIDs) for the 
> underlying FactEntries.  If you change the definition of a FactEntry, it will 
> change that definition for every program point.  So if you do an unlock on 
> one side of a branch, and change the underlying FactEntry to reflect that, 
> then analysis will then think you have also done the unlock on the other side 
> of the branch.  
> 
> Any changes should always be done by adding or removing entries from the 
> FactSet, not by mutating the underlying FactEntries.
> 
That explains why there are problems with back edges. Now that you say it I'm 
actually aware of how the `FactManager` and `FactSet` work, but apparently I 
didn't consider that here.

Would you mind if I make a separate change that returns only `const FactEntry*` 
from the `FactManager`? Maybe that makes it harder to write wrong code in the 
future.


Repository:
  rC Clang

https://reviews.llvm.org/D51187



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

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

[Aaron Puchert via Phabricator via cfe-commits]

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

When passing by reference, we check if the reference is const-qualified
and if it isn't, we demand an exclusive lock. Unlike checking const
qualifiers on member functions, there are probably not many false
positives here: if a function takes a non-const reference, it will
in almost all cases modify the object that we passed it.


Repository:
  rC Clang

https://reviews.llvm.org/D52395

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  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
@@ -5026,19 +5026,19 @@
 
   void test1() {
 copy(foo); // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
-write1(foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
-write2(10, foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+write1(foo);   // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+write2(10, foo);   // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
 read1(foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 read2(10, foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
-destroy(mymove(foo));  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+destroy(mymove(foo));  // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
 
-mwrite1(foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
-mwrite2(10, foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+mwrite1(foo);   // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+mwrite2(10, foo);   // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
 mread1(foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 mread2(10, foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
-smwrite1(foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
-smwrite2(10, foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+smwrite1(foo);   // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+smwrite2(10, foo);   // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
 smread1(foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 smread2(10, foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
@@ -5053,12 +5053,13 @@
 (*this) << foo;  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
 copy(*foop); // expected-warning {{reading the value pointed to by 'foop' requires holding mutex 'mu'}}
-write1(*foop);   // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
-write2(10, *foop);   // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
+write1(*foop);   // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+write2(10, *foop);   // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
 read1(*foop);// expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
 read2(10, *foop);// expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
-destroy(mymove(*foop));  // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
+destroy(mymove(*foop));  // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
 
+// TODO -- sometimes this should warn about writing.
 copy(*f

[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

> Any changes should always be done by adding or removing entries from the 
> FactSet, not by mutating the underlying FactEntries.

To make that clearer in the code, I made `FactEntry`s immutable that are 
managed by `FactManager` in https://reviews.llvm.org/rC342787.

In https://reviews.llvm.org/D51187#1242620, @delesley wrote:

> It should definitely go in -Wthread-safety-beta so it won't break the build.  
> Unfortunately, the before/after checks are still in thread-safety-beta, and 
> they should really be moved out of beta before something else is moved in.  
> The good news is that Matthew O'Niel just volunteered to do that.  That is, 
> unfortunately, not a trivial operation, so it may be some weeks before he's 
> done.


That's Ok for me. It's not something that I terribly need, but it seemed to 
make things more consistent.

Does the migration of the before/after checks include changes to how it is 
handled? Because I wondered why it doesn't work on S-expressions like the rest 
of the analysis, but just `ValueDecl`s. That leads to some false positives with 
more complex expressions:

  class A {
  public:
Mutex mu1;
Mutex mu2 ACQUIRED_AFTER(mu1);
  };
  
  class B {
A a1, a2;
Mutex lm ACQUIRED_AFTER(a1.mu2);
  
  public:
void f() {
  a2.mu2.Lock();
  a1.mu1.Lock();// warns "mutex 'mu1' must be acquired before 'mu2'", 
but they are on different objects
  a1.mu1.Unlock();
  a2.mu2.Unlock();
}
  
void g() {
  lm.Lock();
  a1.mu1.Lock();
  a1.mu1.Unlock();
  a2.mu1.Lock();// Warns "mutex 'mu1' must be acquired before 'lm'", 
but lm talks only about a1.mu2.
  a2.mu1.Unlock();
  lm.Unlock();
}
  };

But maybe that's not the right place to discuss this.


Repository:
  rC Clang

https://reviews.llvm.org/D51187



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

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

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

When people are really sure they'll get the lock they sometimes use
__builtin_expect. It's also used by some assertion implementations.
Asserting that try-lock succeeded is basically the same as asserting
that the lock is not held by anyone else (and acquiring it).


Repository:
  rC Clang

https://reviews.llvm.org/D52398

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
@@ -1754,14 +1754,29 @@
 mu.Unlock();
   }
 
+  void foo2_builtin_expect() {
+if (__builtin_expect(!mu.TryLock(), false))
+  return;
+a = 2;
+mu.Unlock();
+  }
+
   void foo3() {
 bool b = mu.TryLock();
 if (b) {
   a = 3;
   mu.Unlock();
 }
   }
 
+  void foo3_builtin_expect() {
+bool b = mu.TryLock();
+if (__builtin_expect(b, true)) {
+  a = 3;
+  mu.Unlock();
+}
+  }
+
   void foo4() {
 bool b = mu.TryLock();
 if (!b) return;
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -33,6 +33,7 @@
 #include "clang/Analysis/Analyses/ThreadSafetyUtil.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
@@ -1388,8 +1389,11 @@
   if (!Cond)
 return nullptr;
 
-  if (const auto *CallExp = dyn_cast(Cond))
+  if (const auto *CallExp = dyn_cast(Cond)) {
+if (CallExp->getBuiltinCallee() == Builtin::BI__builtin_expect)
+  return getTrylockCallExpr(CallExp->getArg(0), C, Negate);
 return CallExp;
+  }
   else if (const auto *PE = dyn_cast(Cond))
 return getTrylockCallExpr(PE->getSubExpr(), C, Negate);
   else if (const auto *CE = dyn_cast(Cond))


Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1754,14 +1754,29 @@
 mu.Unlock();
   }
 
+  void foo2_builtin_expect() {
+if (__builtin_expect(!mu.TryLock(), false))
+  return;
+a = 2;
+mu.Unlock();
+  }
+
   void foo3() {
 bool b = mu.TryLock();
 if (b) {
   a = 3;
   mu.Unlock();
 }
   }
 
+  void foo3_builtin_expect() {
+bool b = mu.TryLock();
+if (__builtin_expect(b, true)) {
+  a = 3;
+  mu.Unlock();
+}
+  }
+
   void foo4() {
 bool b = mu.TryLock();
 if (!b) return;
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -33,6 +33,7 @@
 #include "clang/Analysis/Analyses/ThreadSafetyUtil.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
@@ -1388,8 +1389,11 @@
   if (!Cond)
 return nullptr;
 
-  if (const auto *CallExp = dyn_cast(Cond))
+  if (const auto *CallExp = dyn_cast(Cond)) {
+if (CallExp->getBuiltinCallee() == Builtin::BI__builtin_expect)
+  return getTrylockCallExpr(CallExp->getArg(0), C, Negate);
 return CallExp;
+  }
   else if (const auto *PE = dyn_cast(Cond))
 return getTrylockCallExpr(PE->getSubExpr(), C, Negate);
   else if (const auto *CE = dyn_cast(Cond))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

[Aaron Puchert via Phabricator via cfe-commits]

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

Instead of only examining call arguments, we also examine constructor
arguments applying the same rules.

That was an oppurtunity for refactoring the examination procedure to
work with iterators instead of integer indices. For the case of
CallExprs no functional change is intended.


Repository:
  rC Clang

https://reviews.llvm.org/D52443

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
@@ -4999,8 +4999,13 @@
 void operator/(const Foo& f, const Foo& g);
 void operator*(const Foo& f, const Foo& g);
 
-
-
+// Test constructors.
+struct FooRead {
+  FooRead(const Foo &);
+};
+struct FooWrite {
+  FooWrite(Foo &);
+};
 
 class Bar {
 public:
@@ -5032,6 +5037,9 @@
 read2(10, foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 destroy(mymove(foo));  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
+FooRead reader(foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+FooWrite writer(foo);  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+
 mwrite1(foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 mwrite2(10, foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 mread1(foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -1534,6 +1534,10 @@
  ProtectedOperationKind POK = POK_VarAccess);
 
   void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr);
+  void ExamineCallArguments(const FunctionDecl *FD,
+CallExpr::const_arg_iterator ArgBegin,
+CallExpr::const_arg_iterator ArgEnd,
+bool OperatorFun);
 
 public:
   BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
@@ -1934,6 +1938,43 @@
   checkAccess(CE->getSubExpr(), AK_Read);
 }
 
+void BuildLockset::ExamineCallArguments(const FunctionDecl *FD,
+CallExpr::const_arg_iterator ArgBegin,
+CallExpr::const_arg_iterator ArgEnd,
+bool OperatorFun) {
+  // Currently we can't do anything if we don't know the function declaration.
+  if (!FD)
+return;
+
+  // NO_THREAD_SAFETY_ANALYSIS does double duty here.  Normally it
+  // only turns off checking within the body of a function, but we also
+  // use it to turn off checking in arguments to the function.  This
+  // could result in some false negatives, but the alternative is to
+  // create yet another attribute.
+  if (FD->hasAttr())
+return;
+
+  const ArrayRef Params = FD->parameters();
+  auto Param = Params.begin();
+  if (OperatorFun) {
+// Ignore the first argument of operators; it's been checked elsewhere.
+++ArgBegin;
+
+// For method operators, the first argument is the implicit self argument,
+// and doesn't appear in the FunctionDecl, but for non-methods it does.
+if (!isa(FD))
+  ++Param;
+  }
+
+  // There can be default arguments, so we stop when one iterator is at end().
+  for (auto Arg = ArgBegin; Param != Params.end() && Arg != ArgEnd;
+   ++Param, ++Arg) {
+QualType Qt = (*Param)->getType();
+if (Qt->isReferenceType())
+  checkAccess(*Arg, AK_Read, POK_PassByRef);
+  }
+}
+
 void BuildLockset::VisitCallExpr(const CallExpr *Exp) {
   bool ExamineArgs = true;
   bool OperatorFun = false;
@@ -1990,41 +2031,8 @@
   }
 
   if (ExamineArgs) {
-if (const FunctionDecl *FD = Exp->getDirectCallee()) {
-  // NO_THREAD_SAFETY_ANALYSIS does double duty here.  Normally it
-  // only turns off checking within the body of a function, but we also
-  // use it to turn off checking in arguments to the function.  This
-  // could result in some false negatives, but the alternative is to
-  // create yet another attribute.
-  if (!FD->hasAttr()) {
-unsigned Fn = FD->getNumParams();
-unsigned Cn = Exp->getNumArgs();
-unsigned Skip = 0;
-
-unsigned i = 0;
-if (OperatorFun) {
-  if (isa(FD)) {
-// First arg in operator call is implicit self argument,
-// and doesn't appear in the FunctionDecl.
-Skip = 1;
-Cn

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

No problem. Thanks for reviewing! I'm terribly sorry to be bombarding the two 
of you with so many review requests lately, and I hope it'll soon be over.


Repository:
  rC Clang

https://reviews.llvm.org/D52398



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

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

There is a (technical) merge conflict between this change and 
https://reviews.llvm.org/D52395, but that shouldn't be of any concern for the 
review. The issues are rather independent. (I think.)


Repository:
  rC Clang

https://reviews.llvm.org/D52443



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

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

While most people probably just use ordinary mutexes, hence won't be affected, 
those that use read/write locks need to know when to use a shared and when to 
use an exclusive lock. What makes things hard in C++ is that through passing by 
non-const reference, an object can actually be modified although it looks like 
it's only passed as a parameter. That makes it really hard to decide whether a 
shared lock is sufficient just by reading the code, which was a major pain 
point in our project. We had to do very careful reviews to make sure we had the 
right kind of lock in every function. I'd like to offload some of that work to 
the compiler.

That's the main reason for this change: it's pretty easy to find out where a 
variable is used, but it's hard to say which uses are reads and which are 
writes in modern C++, if it weren't for const.

In https://reviews.llvm.org/D52395#1244021, @aaron.ballman wrote:

> > Unlike checking const qualifiers on member functions, there are probably 
> > not many false positives here: if a function takes a non-const reference, 
> > it will in almost all cases modify the object that we passed it.
>
> I'm not certain I agree with the predicate here. We can make that inference 
> when there *is* a const qualifier on the parameter, but I don't I think we 
> can make any assumptions about whether it will or won't modify the object 
> passed in in the absence of a const qualifier. This has come up in the past 
> for things like `C(C&)` being a valid copy constructor despite the parameter 
> being non-const.


For const qualifiers on member functions, the concern is that I might have a 
shared lock on a data structure like a std::vector and then I read stuff from 
that vector. But unless I'm reading the vector from a const member function, or 
through a const reference, I'm going to use the non-const `operator[]`. That 
operator doesn't actually modify anything, but it returns a reference that 
allows me to modify stuff. However, I'm not doing any of that, so why should 
there be a warning. I think that's a valid concern.

Effectively we have two overloads that are both not actually modifying 
anything, but the non-const variant returns a reference that does allow us to 
modify something. My thinking was that this pattern is not so common with 
function parameters. The function(s) will return a reference to a part of an 
object, and such references will typically come from member functions for which 
we don't enforce this. So even if I use a non-modifying `` like 
`std::find`, there'll be no warning because the iterators come from container 
members `begin` and `end`. (By the way, that would be easy to fix by using 
`cbegin` and `cend`.)

The way I see it, there are basically three cases of false positives:

- The function doesn't actually modify the object, but takes it as non-const 
reference nevertheless. That can be easily fixed though and I think it's a fix 
that doesn't hurt.
- The function modifies the object sometimes, but here the user is very certain 
that it won't. That might be indication of a poor design, in any event it's 
very fragile. As a developer, I would feel very anxious about this, especially 
in a larger code base.
- We have two overloads of a function, both don't modify the object, but the 
non-const variant returns a non-const reference to some part of the object. 
This isn't backed up by data, but I think this doesn't happen nearly as often 
as with member functions. There are several fixes available as well: one could 
make sure that we only have a const reference there, or make the function a 
const member, or if all else fails, by taking a const reference to the object 
we don't want to modify and then call the function with that as argument. 
That's an additional line of code like `const auto &cref = obj;` which I think 
will rarely be needed.

So basically my argument is that this can catch really nasty bugs, and false 
positives should be rare. If they occur, they can be fixed by making the code 
more const-correct. Such fixes should be easy to sell, because everybody loves 
const-correctness, and it doesn't require any annotations.

> We might need some data to back this assertion up.

Makes sense to me, but I don't have access to the largest code base in terms of 
thread safety analysis. Maybe @delesley can help here? Or I need to apply for a 
job at Google. On our own code, we're still in the very early stages of 
annotating, so I can't provide reliable data from there. I've seen thread 
safety analysis in flutter  and Chromium, 
but both don't seem to use read/write locks.




Comment at: lib/Analysis/ThreadSafety.cpp:2023
   QualType Qt = Pvd->getType();
-  if (Qt->isReferenceType())
-checkAccess(Arg, AK_Read, POK_PassByRef);
+  if (const auto* RT = dyn_cast(&*Qt)) {
+if (RT->getPointeeType().isConstQualified())

[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(&mu);
+  x = 2; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+}
+
+void innerUnlock() {
+  MutexLock outer(&mu);
+  if (x == 0) {
+MutexUnlock inner(&mu);
+x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+  }
+  x = 2;
+}
+
+void manual() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope(&mu);
+  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(&mu);
+  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(&mu);
+  scope.Lock();
+  scope.Lock(); // expected-warning {{acquiring mutex 'mu' that is already held}}
+}
+
+void doubleUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+  MutexUnlock scope(&mu);
+  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(&mu, &other);
+  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 &CE, SourceLocation Loc,
-  const CapExprSet &Excl, const CapExprSet &Shrd)
+  const CapExprSet &Excl, const CapExprSet &Shrd,
+  const CapExprSet &ExclRel, const CapExprSet &ShrdRel)
   : FactEntry(CE, LK_Exclusive, Loc, false) {
 for (const auto &M : Excl)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
 for (const auto &M : Shrd)
-  UnderlyingMutexes.push_back(M.sexpr());
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
+for (const auto &M : ExclRel)
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
+for (const auto &M : ShrdRel)
+  UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
   }
 
   void
   handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
 So

[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] D52443: Thread safety analysis: Examine constructor arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: lib/Analysis/ThreadSafety.cpp:1970
+  // There can be default arguments, so we stop when one iterator is at end().
+  for (auto Arg = ArgBegin; Param != Params.end() && Arg != ArgEnd;
+   ++Param, ++Arg) {

aaron.ballman wrote:
> How should this interact with variadic functions? Either ones that use `...` 
> with a C varargs function, or ones that use parameter packs in C++. (I 
> realize this is basically existing code, but the question remains.)
For instantiated variadic templates we match against the instantiation of the 
template, so we can match the parameters just as for an ordinary function. My 
understanding is that the thread safety analysis doesn't run over templates, 
only instantiations, so that should be fine.

With C variadic arguments we should have a shorter parameter list, so we don't 
check the matching variadic arguments. However, if I'm not mistakten, the 
variadic arguments are passed by value, so the reference checks aren't needed.

Maybe I can add some tests for these cases.



Comment at: lib/Analysis/ThreadSafety.cpp:2050
+  } else {
+ExamineCallArguments(D, Exp->arg_begin(), Exp->arg_end(), false);
   }

aaron.ballman wrote:
> Can you add a comment for the bool argument? e.g., `/*OperatorFun*/false`
Depending on `OperatorFun`, we shift some of the iterators. We could also do 
that on the caller site. I'll see if that looks better.


Repository:
  rC Clang

https://reviews.llvm.org/D52443



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

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

I've thought about your concerns and the important thing is: I didn't actually 
test this on a larger code base. The change makes sense to me, but maybe I'm 
missing something. So maybe instead of pushing through further restrictions, I 
should focus on rolling out the analysis on our own messy code base. Maybe I'm 
going to see the false positives right there.

However, as messy as our code may be, it might be messy in different ways than 
Google's code. So I might not see the same issues. Then on the other hand, 
there might be (probably smaller) code bases which don't have a problem with 
stricter rules. So I think additional flags are probably not the worst idea, 
but we need to keep their number small.

> Second, there needs to be a thread-safety-variant of "const_cast" -- i.e. a 
> way to pass a reference to a function without triggering the warning.  That 
> may already be possible via clever use of our current annotations, or you may 
> have to fiddle with something, but it needs to appear in the test cases.

There is such a mechanism in the test suite. It uses that we don't check the 
arguments for functions annotated with `no_thread_safety_analysis`, introduced 
by r246806 :

  // Takes a reference to a guarded data member, and returns an unguarded
  // reference.
  template 
  inline const T& ts_unchecked_read(const T& v) NO_THREAD_SAFETY_ANALYSIS {
return v;
  }
  
  template 
  inline T& ts_unchecked_read(T& v) NO_THREAD_SAFETY_ANALYSIS {
return v;
  }


Repository:
  rC Clang

https://reviews.llvm.org/D52395



___
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 &M : 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] D52443: Thread safety analysis: Examine constructor arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 167856.
aaronpuchert marked 4 inline comments as done.
aaronpuchert added a comment.

Moved iterator shifting to VisitCallExpr, eliminated boolean variables, and 
minor corrections as suggested in the review. There remains no intended 
functional change to VisitCallExpr.


Repository:
  rC Clang

https://reviews.llvm.org/D52443

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
@@ -4982,6 +4982,8 @@
   void operator+(const Foo& f);
 
   void operator[](const Foo& g);
+
+  void operator()();
 };
 
 template
@@ -4999,8 +5001,23 @@
 void operator/(const Foo& f, const Foo& g);
 void operator*(const Foo& f, const Foo& g);
 
+// Test constructors.
+struct FooRead {
+  FooRead(const Foo &);
+};
+struct FooWrite {
+  FooWrite(Foo &);
+};
 
+// Test variadic functions
+template
+void copyVariadic(T...) {}
+template
+void writeVariadic(T&...) {}
+template
+void readVariadic(const T&...) {}
 
+void copyVariadicC(int, ...);
 
 class Bar {
 public:
@@ -5032,6 +5049,14 @@
 read2(10, foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 destroy(mymove(foo));  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
+copyVariadic(foo); // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
+readVariadic(foo); // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+writeVariadic(foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+copyVariadicC(1, foo); // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
+
+FooRead reader(foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+FooWrite writer(foo);  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+
 mwrite1(foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 mwrite2(10, foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 mread1(foo);// expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
@@ -5050,6 +5075,7 @@
  // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}}
 foo[foo2];   // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \
  // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}}
+foo();   // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
 (*this) << foo;  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
 copy(*foop); // expected-warning {{reading the value pointed to by 'foop' requires holding mutex 'mu'}}
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -1534,6 +1534,10 @@
  ProtectedOperationKind POK = POK_VarAccess);
 
   void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr);
+  void examineArguments(const FunctionDecl *FD,
+CallExpr::const_arg_iterator ArgBegin,
+CallExpr::const_arg_iterator ArgEnd,
+bool SkipFirstParam = false);
 
 public:
   BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
@@ -1934,10 +1938,37 @@
   checkAccess(CE->getSubExpr(), AK_Read);
 }
 
-void BuildLockset::VisitCallExpr(const CallExpr *Exp) {
-  bool ExamineArgs = true;
-  bool OperatorFun = false;
+void BuildLockset::examineArguments(const FunctionDecl *FD,
+CallExpr::const_arg_iterator ArgBegin,
+CallExpr::const_arg_iterator ArgEnd,
+bool SkipFirstParam) {
+  // Currently we can't do anything if we don't know the function declaration.
+  if (!FD)
+return;
+
+  // NO_THREAD_SAFETY_ANALYSIS does double duty here.  Normally it
+  // only turns off checking within the body of a function, but we also
+  // use it to turn off checking in arguments to the function.  This
+  // could result in some false negatives, but the alternative is to
+  // create yet another attribute.
+  if (FD->hasAttr())
+return;
+
+  const ArrayRef Params = FD->parameters();
+  auto Param = Params.begin();
+  if (SkipFirstParam)
+++Param;
+
+  // There can be default arguments, so we stop when one iterator is a

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked 2 inline comments as done.
aaronpuchert added a comment.

Since the (remaining) arguments are examined in a separate function, I thought 
I'd eliminate the boolean variables in `VisitCallExpr`. Apparently I prefer 
control flow over booleans, but if you disagree I can obviously change it back.




Comment at: lib/Analysis/ThreadSafety.cpp:2046
   const CXXConstructorDecl *D = Exp->getConstructor();
   if (D && D->isCopyConstructor()) {
 const Expr* Source = Exp->getArg(0);

delesley wrote:
> As a side note, we should probably special-case the move constructor too, 
> with AK_Written.  That should be in a separate patch, though, and needs to be 
> sequestered under -Wthread-safety-beta, which is complicated.   
I think your arguments from D52395 apply here as well: the move constructor 
does not necessarily write. Many simple types are trivially move constructible, 
and then the move constructor is effectively the same as the copy constructor.


Repository:
  rC Clang

https://reviews.llvm.org/D52443



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

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

@delesley Any objections to this?

It's certainly useful for our code base, because our `assert`-like macros use 
`__builtin_expect`, but I'm not sure if that applies to the general public.


Repository:
  rC Clang

https://reviews.llvm.org/D52398



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

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL343681: Thread safety analysis: Unwrap __builtin_expect in 
getTrylockCallExpr (authored by aaronpuchert, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D52398

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


Index: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1754,14 +1754,29 @@
 mu.Unlock();
   }
 
+  void foo2_builtin_expect() {
+if (__builtin_expect(!mu.TryLock(), false))
+  return;
+a = 2;
+mu.Unlock();
+  }
+
   void foo3() {
 bool b = mu.TryLock();
 if (b) {
   a = 3;
   mu.Unlock();
 }
   }
 
+  void foo3_builtin_expect() {
+bool b = mu.TryLock();
+if (__builtin_expect(b, true)) {
+  a = 3;
+  mu.Unlock();
+}
+  }
+
   void foo4() {
 bool b = mu.TryLock();
 if (!b) return;
Index: cfe/trunk/lib/Analysis/ThreadSafety.cpp
===
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp
@@ -33,6 +33,7 @@
 #include "clang/Analysis/Analyses/ThreadSafetyUtil.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
@@ -1388,8 +1389,11 @@
   if (!Cond)
 return nullptr;
 
-  if (const auto *CallExp = dyn_cast(Cond))
+  if (const auto *CallExp = dyn_cast(Cond)) {
+if (CallExp->getBuiltinCallee() == Builtin::BI__builtin_expect)
+  return getTrylockCallExpr(CallExp->getArg(0), C, Negate);
 return CallExp;
+  }
   else if (const auto *PE = dyn_cast(Cond))
 return getTrylockCallExpr(PE->getSubExpr(), C, Negate);
   else if (const auto *CE = dyn_cast(Cond))


Index: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1754,14 +1754,29 @@
 mu.Unlock();
   }
 
+  void foo2_builtin_expect() {
+if (__builtin_expect(!mu.TryLock(), false))
+  return;
+a = 2;
+mu.Unlock();
+  }
+
   void foo3() {
 bool b = mu.TryLock();
 if (b) {
   a = 3;
   mu.Unlock();
 }
   }
 
+  void foo3_builtin_expect() {
+bool b = mu.TryLock();
+if (__builtin_expect(b, true)) {
+  a = 3;
+  mu.Unlock();
+}
+  }
+
   void foo4() {
 bool b = mu.TryLock();
 if (!b) return;
Index: cfe/trunk/lib/Analysis/ThreadSafety.cpp
===
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp
@@ -33,6 +33,7 @@
 #include "clang/Analysis/Analyses/ThreadSafetyUtil.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
@@ -1388,8 +1389,11 @@
   if (!Cond)
 return nullptr;
 
-  if (const auto *CallExp = dyn_cast(Cond))
+  if (const auto *CallExp = dyn_cast(Cond)) {
+if (CallExp->getBuiltinCallee() == Builtin::BI__builtin_expect)
+  return getTrylockCallExpr(CallExp->getArg(0), C, Negate);
 return CallExp;
+  }
   else if (const auto *PE = dyn_cast(Cond))
 return getTrylockCallExpr(PE->getSubExpr(), C, Negate);
   else if (const auto *CE = dyn_cast(Cond))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In https://reviews.llvm.org/D52398#1255148, @hokein wrote:

> Hi, @aaronpuchert, the patch has caused many new warnings in our internal 
> codebase, below is a reduced one. Do you mind reverting the patch?
>
>   // if the condition is not true, CHECK will terminate the program.
>   #define CHECK(condition) (__builtin_expect((condition), true)) ? 1 : 0
>  
>   void foo() {
>   CHECK(mu.TryLock()); 
>   mu.Unlock();
>   }
>


Even before there was another warning in this code: "releasing mutex 'mu' that 
was not held" on `mu.Unlock()`. Is there an example that didn't show any 
warnings before?

And can you confirm that the warning also appeared before if you replace 
`__builtin_expect(x, *)` by `x`? Both are functionally equivalent.

I will have a closer look, but my first feeling is that the problem is //not// 
that we unwrap __builtin_expect, but instead that we don't consider the ternary 
operator `?:` in `getTrylockCallExpr`. I'll see if I can fix that.


Repository:
  rL LLVM

https://reviews.llvm.org/D52398



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

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

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

We unwrap conditional expressions containing try-lock functions.

Additionally we don't branch on conditionals, since that is usually not
helpful. When joining the branches we would almost certainly get a
warning otherwise.

Hopefully fixes an issue that was raised in https://reviews.llvm.org/D52398.


Repository:
  rC Clang

https://reviews.llvm.org/D52888

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
@@ -1873,6 +1873,23 @@
int i = a;
mu.Unlock();
   }
+
+  // Test with conditional operator
+  void foo13() {
+if (mu.TryLock() ? 1 : 0)
+  mu.Unlock();
+  }
+
+  void foo14() {
+if (mu.TryLock() ? 0 : 1)
+  return;
+mu.Unlock();
+  }
+
+  void foo15() {
+if (mu.TryLock() ? 0 : 1) // expected-note{{mutex acquired here}}
+  mu.Unlock();// expected-warning{{releasing mutex 'mu' that 
was not held}}
+  }   // expected-warning{{mutex 'mu' is not held on 
every path through here}}
 };  // end TestTrylock
 
 } // end namespace TrylockTest
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -1435,6 +1435,17 @@
 if (BOP->getOpcode() == BO_LOr)
   return getTrylockCallExpr(BOP->getRHS(), C, Negate);
 return nullptr;
+  } else if (const auto *COP = dyn_cast(Cond)) {
+bool TCond, FCond;
+if (getStaticBooleanValue(COP->getTrueExpr(), TCond) &&
+getStaticBooleanValue(COP->getFalseExpr(), FCond)) {
+  if (TCond && !FCond)
+return getTrylockCallExpr(COP->getCond(), C, Negate);
+  if (!TCond && FCond) {
+Negate = !Negate;
+return getTrylockCallExpr(COP->getCond(), C, Negate);
+  }
+}
   }
   return nullptr;
 }
@@ -1449,7 +1460,8 @@
   Result = ExitSet;
 
   const Stmt *Cond = PredBlock->getTerminatorCondition();
-  if (!Cond)
+  // We don't branch on ?:, we branch when the result is used.
+  if (!Cond || isa(PredBlock->getTerminator()))
 return;
 
   bool Negate = false;


Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1873,6 +1873,23 @@
int i = a;
mu.Unlock();
   }
+
+  // Test with conditional operator
+  void foo13() {
+if (mu.TryLock() ? 1 : 0)
+  mu.Unlock();
+  }
+
+  void foo14() {
+if (mu.TryLock() ? 0 : 1)
+  return;
+mu.Unlock();
+  }
+
+  void foo15() {
+if (mu.TryLock() ? 0 : 1) // expected-note{{mutex acquired here}}
+  mu.Unlock();// expected-warning{{releasing mutex 'mu' that was not held}}
+  }   // expected-warning{{mutex 'mu' is not held on every path through here}}
 };  // end TestTrylock
 
 } // end namespace TrylockTest
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -1435,6 +1435,17 @@
 if (BOP->getOpcode() == BO_LOr)
   return getTrylockCallExpr(BOP->getRHS(), C, Negate);
 return nullptr;
+  } else if (const auto *COP = dyn_cast(Cond)) {
+bool TCond, FCond;
+if (getStaticBooleanValue(COP->getTrueExpr(), TCond) &&
+getStaticBooleanValue(COP->getFalseExpr(), FCond)) {
+  if (TCond && !FCond)
+return getTrylockCallExpr(COP->getCond(), C, Negate);
+  if (!TCond && FCond) {
+Negate = !Negate;
+return getTrylockCallExpr(COP->getCond(), C, Negate);
+  }
+}
   }
   return nullptr;
 }
@@ -1449,7 +1460,8 @@
   Result = ExitSet;
 
   const Stmt *Cond = PredBlock->getTerminatorCondition();
-  if (!Cond)
+  // We don't branch on ?:, we branch when the result is used.
+  if (!Cond || isa(PredBlock->getTerminator()))
 return;
 
   bool Negate = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

@hokein Please have a look at https://reviews.llvm.org/D52888, maybe you can 
try it out already. The problem was that `?:` expressions are considered a 
branch point and when merging both branches the warning was emitted. Before 
this change, we couldn't look into `__builtin_expect` and so we didn't see that 
the result of `TryLock` is used to branch. Take this source:

  void foo() {
  if (mu.TryLock() ? 1 : 0)
mu.Unlock();
  }

The CFG is:

  void foo()
   [B6 (ENTRY)]
 Succs (1): B5
  
   [B1]
 1: mu
 2: [B1.1].Unlock
 3: [B1.2]()
 Preds (1): B2
 Succs (1): B0
  
   [B2]
 1: [B5.3] ? [B3.1] : [B4.1]
 2: [B2.1] (ImplicitCastExpr, IntegralToBoolean, _Bool)
 T: if [B2.2]
 Preds (2): B3 B4
 Succs (2): B1 B0
  
   [B3]
 1: 1
 Preds (1): B5
 Succs (1): B2
  
   [B4]
 1: 0
 Preds (1): B5
 Succs (1): B2
  
   [B5]
 1: mu
 2: [B5.1].TryLock
 3: [B5.2]()
 T: [B5.3] ? ... : ...
 Preds (1): B6
 Succs (2): B3 B4
  
   [B0 (EXIT)]
 Preds (2): B1 B2

So we start in `B5`, then call `TryLock` and branch on `?:` into `B3` or `B4`. 
We find that in `B3` the lock is held and in `B4` it isn't, so when both merge 
into `B2` we complain that we don't know if the lock is held or not.

I think the proper solution is to not branch on conditionals, and rather 
introspect them when a "real" branch happens. There is a slight possibility 
that this might break other use cases, but I hope not.


Repository:
  rL LLVM

https://reviews.llvm.org/D52398



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

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

By the way, the tests also work with `__builtin_expect`, but I didn't want to 
blow up the code too much.


Repository:
  rC Clang

https://reviews.llvm.org/D52888



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

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: lib/Analysis/ThreadSafety.cpp:1445
+  if (!TCond && FCond) {
+Negate = !Negate;
+return getTrylockCallExpr(COP->getCond(), C, Negate);

aaron.ballman wrote:
> Rather than do an assignment here, why not just pass `!Negate` directly 
> below, since you're returning?
The third parameter of `getTrylockCallExpr` is a (non-const) reference, so I 
can only pass an lvalue. It's basically a 
[call-by-reference](https://en.wikipedia.org/wiki/Evaluation_strategy#Call_by_reference)
 pattern.



Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:1879-1880
+  void foo13() {
+if (mu.TryLock() ? 1 : 0)
+  mu.Unlock();
+  }

aaron.ballman wrote:
> Can you add a test that shows we get it right even if the user does something 
> less than practical, like:
> ```
> if (mu.TryLock() ? mu.TryLock() : false); // Warn about double lock
> if (mu.TryLock() ? mu.Unlock(), 1 : 0)
>   mu.Unlock(); // Warn about unlocking unheld lock
> if (mu.TryLock() ? 1 : mu.Unlock(), 0)
>   mu.Unlock(); // Warn about unlocking an unheld lock
> if (mu.TryLock() ? (true ? mu.TryLock() : false) : false); // Warn about 
> double lock
> ```
I'm afraid these don't work as expected if we don't branch on `?:` as before. 
Basically we treat this as if both conditions where evaluated unconditionally.

On calling a try-lock function, no lock is immediately acquired. Only when we 
branch on the result of that try-lock call is the mutex added to the capability 
set on the appropriate branch. Now if we branch on `?:` (the situation before 
this change), we are going to complain about conditionally held locks on the 
join point in @hokein's use case, and if we don't branch (the behavior after 
this change), we are not treating your examples correctly. I'm not sure we can 
treat both as intended.

I'm slightly leaning towards not branching: the C++ standard says that only the 
[used expression is actually 
evaluated](https://en.cppreference.com/w/cpp/language/operator_other#Conditional_operator),
 but I think it's not considered good practice to have side effects in a 
ternary operator. (I can't actually find it anywhere besides a discussion on 
[Hacker News](https://news.ycombinator.com/item?id=14810994). Personally I 
would prefer to use an if-statement if one of the expressions has side-effects.)

Tell me what you think.


Repository:
  rC Clang

https://reviews.llvm.org/D52888



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