[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D84604#2364568 , @aaronpuchert 
wrote:

> In D84604#2363768 , @rupprecht wrote:
>
>> I applied D87194  locally and rebuilt the 
>> original source, and not only am I seeing the original issue (also firing on 
>> `DoThings()` when it should only be on `DoStuff()`), I'm also seeing: 
>> `error: acquiring mutex 'lock' requires negative capability '!lock' 
>> [-Werror,-Wthread-safety-negative]`, where `lock` is a local variable, 
>> defined as `MutexLock lock(mutex_)`.
>
> Oh yes, I need to rebase this, sorry if I wasted your time. This is still on 
> top of the bug that @lebedev.ri pointed out in D84604#2262745 
>  and on top of the bug that you 
> pointed out.

No worries, it was only a minute to patch, and then a few more minutes to take 
a snack break while it rebuilt :)

>> I'll work on getting a better repro for this.
>
> Maybe wait a bit with that, I'll add you as reviewer when I've done the 
> rebase and then you can try it again. I hope to have covered both locals and 
> static members now.
>
> Another issue is linkage, but I'll have to read up on that a bit.

Yep, I'm not qualified to actually review the code, but I'd be happy to test 
any patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D84604#2363768 , @rupprecht wrote:

> I applied D87194  locally and rebuilt the 
> original source, and not only am I seeing the original issue (also firing on 
> `DoThings()` when it should only be on `DoStuff()`), I'm also seeing: `error: 
> acquiring mutex 'lock' requires negative capability '!lock' 
> [-Werror,-Wthread-safety-negative]`, where `lock` is a local variable, 
> defined as `MutexLock lock(mutex_)`.

Oh yes, I need to rebase this, sorry if I wasted your time. This is still on 
top of the bug that @lebedev.ri pointed out in D84604#2262745 
 and on top of the bug that you 
pointed out.

> I'll work on getting a better repro for this.

Maybe wait a bit with that, I'll add you as reviewer when I've done the rebase 
and then you can try it again. I hope to have covered both locals and static 
members now.

Another issue is linkage, but I'll have to read up on that a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D84604#2363445 , @aaronpuchert 
wrote:

> Pushed a fix in rGbbed8cfe80cd27d3a47d877c7608d9be4e487d97 
> . For 
> now we just consider all static members as inaccessible, so we'll treat them 
> as we did before this change.

I can confirm this fixes the breakage -- thanks!

> I have proposed making the check stronger for non-static members in D87194 
> , perhaps it makes sense to extend this to 
> static members as well so that it fires on `DoStuff()` again.

I applied D87194  locally and rebuilt the 
original source, and not only am I seeing the original issue (also firing on 
`DoThings()` when it should only be on `DoStuff()`), I'm also seeing: `error: 
acquiring mutex 'lock' requires negative capability '!lock' 
[-Werror,-Wthread-safety-negative]`, where `lock` is a local variable, defined 
as `MutexLock lock(mutex_)`.

I'll work on getting a better repro for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Pushed a fix in rGbbed8cfe80cd27d3a47d877c7608d9be4e487d97 
. For now 
we just consider all static members as inaccessible, so we'll treat them as we 
did before this change.

I have proposed making the check stronger for non-static members in D87194 
, perhaps it makes sense to extend this to 
static members as well so that it fires on `DoStuff()` again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D84604#2363134 , @rupprecht wrote:

> I'm seeing failures which I think are due to this patch -- I don't have a 
> nice godbolt repro yet, but it's something like:

Yes, that's very likely.

> I'm also seeing the same error for `DoThings()`, which doesn't make sense. 
> When I add it, I then get errors that `mu_` is private, and therefore needs 
> to be made public or friended for the thread analysis -- which is not at all 
> a good change (the mutex should be encapsulated in the class). Even though 
> it's "global" in the linkage sense, it's not visible outside of the class.

I'll see if I can fix this quickly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I'm seeing failures which I think are due to this patch -- I don't have a nice 
godbolt repro yet, but it's something like:

  foo.h: 
  class Foo {
   public:
static void DoStuff();  // Grabs mu_
  
  private:
static std::vector blah_ GUARDED_BY(mu_);
static Mutex mu_;
  };
  
  bar.h:
  class Bar {
static void DoThings(); // calls Foo::DoStuff()
  };

Because `DoStuff()` grabs `mu_`, it needs the lock, and this patch gives an 
error like `requires negative capability 'mu_'`. That's fine, and in fact 
better analysis is welcome.

However, I'm also seeing the same error for `DoThings()`, which doesn't make 
sense. When I add it, I then get errors that `mu_` is private, and therefore 
needs to be made public or friended for the thread analysis -- which is not at 
all a good change (the mutex should be encapsulated in the class). Even though 
it's "global" in the linkage sense, it's not visible outside of the class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5250a03a9959: Thread safety analysis: Consider global 
variables in scope (authored by aaronpuchert).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -21,6 +21,12 @@
   void AssertReaderHeld() ASSERT_SHARED_LOCK();
 };
 
+class SCOPED_LOCKABLE MutexLock {
+public:
+  MutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  MutexLock(Mutex *mu, bool adopt) EXCLUSIVE_LOCKS_REQUIRED(mu);
+  ~MutexLock() UNLOCK_FUNCTION();
+};
 
 namespace SimpleTest {
 
@@ -77,10 +83,43 @@
 mu.Unlock();
 baz();   // no warning -- !mu in set.
   }
+
+  void test4() {
+MutexLock lock(); // expected-warning {{acquiring mutex 'mu' requires negative capability '!mu'}}
+  }
 };
 
 }  // end namespace SimpleTest
 
+Mutex globalMutex;
+
+namespace ScopeTest {
+
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+void fq() EXCLUSIVE_LOCKS_REQUIRED(!::globalMutex);
+
+namespace ns {
+  Mutex globalMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+  void fq() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex);
+}
+
+void testGlobals() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex) {
+  f(); // expected-warning {{calling function 'f' requires negative capability '!globalMutex'}}
+  fq();// expected-warning {{calling function 'fq' requires negative capability '!globalMutex'}}
+  ns::f();
+  ns::fq();
+}
+
+void testNamespaceGlobals() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex) {
+  f();
+  fq();
+  ns::f();  // expected-warning {{calling function 'f' requires negative capability '!globalMutex'}}
+  ns::fq(); // expected-warning {{calling function 'fq' requires negative capability '!globalMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) __attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+__attribute__((requires_capability(!FlightControl))) {
   const char *msg;
 
   while ((msg = deque_log_msg())) {
@@ -5044,13 +5045,13 @@
   }
 }
 
-void spawn_fake_logger_thread(void) {
+void spawn_fake_logger_thread(void) __attribute__((requires_capability(!FlightControl))) {
   acquire(Logger);
   logger_entry();
   release(Logger);
 }
 
-int main(void) {
+int main(void) __attribute__((requires_capability(!FlightControl))) {
   spawn_fake_flight_control_thread();
   spawn_fake_logger_thread();
 
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1266,13 +1266,24 @@
 }
 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr ) {
-  if (!CurrentMethod)
+  const threadSafety::til::SExpr *SExp = CapE.sexpr();
+  assert(SExp && "Null expressions should be ignored");
+
+  // Global variables are always in scope.
+  if (const auto *LP = dyn_cast(SExp)) {
+const ValueDecl *VD = LP->clangDecl();
+return VD->isDefinedOutsideFunctionOrMethod();
+  }
+
+  // Members are in scope from methods of the same class.
+  if (const auto *P = dyn_cast(SExp)) {
+if (!CurrentMethod)
   return false;
-  if (const auto *P = dyn_cast_or_null(CapE.sexpr())) {
 const auto *VD = P->clangDecl();
 if (VD)
   return VD->getDeclContext() == CurrentMethod->getDeclContext();
   }
+
   return false;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Thanks.
Note that i didn't check that this doesn't cause any new false-positives




Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87-89
+  void test4() {
+MutexLock lock(); // expected-warning {{acquiring mutex 'mu' requires 
negative capability '!mu'}}
+  }

aaronpuchert wrote:
> lebedev.ri wrote:
> > aaronpuchert wrote:
> > > @lebedev.ri, I think that's pretty much your case. On my original change, 
> > > this would have also warned about `scope`, not just `mu`.
> > I think i'm missing the point here.
> > I originally reverted this because the diagnostics i was seeing were pretty 
> > unambiguously )to me) bogus.
> > But the only test change since then ensures that diagnostic is emitted in 
> > some case,
> > there are no tests to ensure it is not emitted in some cases.
> > So either my revert was wrong, or this still is either issuing seemingly 
> > bogus diagnostic,
> > or lacks test coverage that it doesn't issue said diagnostic.
> > 
> > Which one is it?
> This test fails on the original, reverted change as follows:
> 
> ```
> error: 'warning' diagnostics seen but not expected: 
>   File [...]/clang/test/SemaCXX/warn-thread-safety-negative.cpp Line 88: 
> acquiring mutex 'lock' requires negative capability '!lock'
> 1 error generated.
> ```
> 
> Maybe you're not familiar with the `-verify` mechanism: it doesn't just check 
> that the expected errors/warnings/notes are emitted, it also checks that 
> nothing more is emitted.
> This test fails on the original, reverted change as follows:

Perfect, thanks for checking/confirming!

> Maybe you're not familiar with the -verify mechanism: it doesn't just check 
> that the expected errors/warnings/notes are emitted, it also checks that 
> nothing more is emitted.

I was aware of that, but did not recall that detail until now. Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87-89
+  void test4() {
+MutexLock lock(); // expected-warning {{acquiring mutex 'mu' requires 
negative capability '!mu'}}
+  }

lebedev.ri wrote:
> aaronpuchert wrote:
> > @lebedev.ri, I think that's pretty much your case. On my original change, 
> > this would have also warned about `scope`, not just `mu`.
> I think i'm missing the point here.
> I originally reverted this because the diagnostics i was seeing were pretty 
> unambiguously )to me) bogus.
> But the only test change since then ensures that diagnostic is emitted in 
> some case,
> there are no tests to ensure it is not emitted in some cases.
> So either my revert was wrong, or this still is either issuing seemingly 
> bogus diagnostic,
> or lacks test coverage that it doesn't issue said diagnostic.
> 
> Which one is it?
This test fails on the original, reverted change as follows:

```
error: 'warning' diagnostics seen but not expected: 
  File [...]/clang/test/SemaCXX/warn-thread-safety-negative.cpp Line 88: 
acquiring mutex 'lock' requires negative capability '!lock'
1 error generated.
```

Maybe you're not familiar with the `-verify` mechanism: it doesn't just check 
that the expected errors/warnings/notes are emitted, it also checks that 
nothing more is emitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87-89
+  void test4() {
+MutexLock lock(); // expected-warning {{acquiring mutex 'mu' requires 
negative capability '!mu'}}
+  }

aaronpuchert wrote:
> @lebedev.ri, I think that's pretty much your case. On my original change, 
> this would have also warned about `scope`, not just `mu`.
I think i'm missing the point here.
I originally reverted this because the diagnostics i was seeing were pretty 
unambiguously )to me) bogus.
But the only test change since then ensures that diagnostic is emitted in some 
case,
there are no tests to ensure it is not emitted in some cases.
So either my revert was wrong, or this still is either issuing seemingly bogus 
diagnostic,
or lacks test coverage that it doesn't issue said diagnostic.

Which one is it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Analysis/ThreadSafety.cpp:1275
+const ValueDecl *VD = LP->clangDecl();
+return VD->isDefinedOutsideFunctionOrMethod();
+  }

aaron.ballman wrote:
> Hmm, I've not seen that function used a whole lot before, but looking at the 
> implementation of it, I think it does what we need it to do here. FWIW, I was 
> expecting something more like this:
> ```
> if (const DeclContext *DC = VD->getLexicalDeclContext())
>   return !DC->getRedeclContext()->isFunctionOrMethod();
> ```
> But I'm not certain if this would ever give a different answer from your 
> approach.
Never seen it before as well, but it does a `DC = DC->getParent()` loop, so 
there is probably a difference. Think about `DC` being a local struct or 
Objective-C block declaration.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87-89
+  void test4() {
+MutexLock lock(); // expected-warning {{acquiring mutex 'mu' requires 
negative capability '!mu'}}
+  }

@lebedev.ri, I think that's pretty much your case. On my original change, this 
would have also warned about `scope`, not just `mu`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Please can you point me where you've added the negative test for the 
false-positive issue that caused the revert last time?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/Analysis/ThreadSafety.cpp:1275
+const ValueDecl *VD = LP->clangDecl();
+return VD->isDefinedOutsideFunctionOrMethod();
+  }

Hmm, I've not seen that function used a whole lot before, but looking at the 
implementation of it, I think it does what we need it to do here. FWIW, I was 
expecting something more like this:
```
if (const DeclContext *DC = VD->getLexicalDeclContext())
  return !DC->getRedeclContext()->isFunctionOrMethod();
```
But I'm not certain if this would ever give a different answer from your 
approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

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

`LiteralPtr`s aren't always globals, local variables are also translated that 
way. So we ask the stored `ValueDecl` if it `isDefinedOutsideFunctionOrMethod`. 
That seems like the right thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -21,6 +21,12 @@
   void AssertReaderHeld() ASSERT_SHARED_LOCK();
 };
 
+class SCOPED_LOCKABLE MutexLock {
+public:
+  MutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  MutexLock(Mutex *mu, bool adopt) EXCLUSIVE_LOCKS_REQUIRED(mu);
+  ~MutexLock() UNLOCK_FUNCTION();
+};
 
 namespace SimpleTest {
 
@@ -77,10 +83,43 @@
 mu.Unlock();
 baz();   // no warning -- !mu in set.
   }
+
+  void test4() {
+MutexLock lock(); // expected-warning {{acquiring mutex 'mu' requires negative capability '!mu'}}
+  }
 };
 
 }  // end namespace SimpleTest
 
+Mutex globalMutex;
+
+namespace ScopeTest {
+
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+void fq() EXCLUSIVE_LOCKS_REQUIRED(!::globalMutex);
+
+namespace ns {
+  Mutex globalMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+  void fq() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex);
+}
+
+void testGlobals() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex) {
+  f(); // expected-warning {{calling function 'f' requires negative capability '!globalMutex'}}
+  fq();// expected-warning {{calling function 'fq' requires negative capability '!globalMutex'}}
+  ns::f();
+  ns::fq();
+}
+
+void testNamespaceGlobals() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex) {
+  f();
+  fq();
+  ns::f();  // expected-warning {{calling function 'f' requires negative capability '!globalMutex'}}
+  ns::fq(); // expected-warning {{calling function 'fq' requires negative capability '!globalMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) __attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+__attribute__((requires_capability(!FlightControl))) {
   const char *msg;
 
   while ((msg = deque_log_msg())) {
@@ -5044,13 +5045,13 @@
   }
 }
 
-void spawn_fake_logger_thread(void) {
+void spawn_fake_logger_thread(void) __attribute__((requires_capability(!FlightControl))) {
   acquire(Logger);
   logger_entry();
   release(Logger);
 }
 
-int main(void) {
+int main(void) __attribute__((requires_capability(!FlightControl))) {
   spawn_fake_flight_control_thread();
   spawn_fake_logger_thread();
 
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1266,13 +1266,24 @@
 }
 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr ) {
-  if (!CurrentMethod)
+  const threadSafety::til::SExpr *SExp = CapE.sexpr();
+  assert(SExp && "Null expressions should be ignored");
+
+  // Global variables are always in scope.
+  if (const auto *LP = dyn_cast(SExp)) {
+const ValueDecl *VD = LP->clangDecl();
+return VD->isDefinedOutsideFunctionOrMethod();
+  }
+
+  // Members are in scope from methods of the same class.
+  if (const auto *P = dyn_cast(SExp)) {
+if (!CurrentMethod)
   return false;
-  if (const auto *P = dyn_cast_or_null(CapE.sexpr())) {
 const auto *VD = P->clangDecl();
 if (VD)
   return VD->getDeclContext() == CurrentMethod->getDeclContext();
   }
+
   return false;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

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

Almost forgot about that. I think I've figured it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D84604#2262745 , @lebedev.ri wrote:

> I'm not sure this is the problematic patch, but i'm now seeing some weird 
> warnings:
>
>   :304:15: warning: acquiring mutex 'guard' requires negative 
> capability '!guard' [-Wthread-safety-negative]
> MutexLocker guard();
> ^
>   :309:15: warning: acquiring mutex 'guard' requires negative 
> capability '!guard' [-Wthread-safety-negative]
> MutexLocker guard();
> ^
>   :322:15: warning: acquiring mutex 'guard' requires negative 
> capability '!guard' [-Wthread-safety-negative]
> MutexLocker guard();
> ^
>   3 warnings generated.
>
> https://godbolt.org/z/5zYT55
>
> If this is the patch that causes it, then i think it's an obvious 
> false-positive (wasn't this patch supposed to only handle globals?),
> if not, the warning's wording is not great.

I've verified that this is indeed the change that causes it,
and therefore gone ahead and temporairly reverted it in 
rG8427885e27813c457dccb011f65e8ded7e31 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I'm not sure this is the problematic patch, but i'm now seeing some weird 
warnings:

  :304:15: warning: acquiring mutex 'guard' requires negative 
capability '!guard' [-Wthread-safety-negative]
MutexLocker guard();
^
  :309:15: warning: acquiring mutex 'guard' requires negative 
capability '!guard' [-Wthread-safety-negative]
MutexLocker guard();
^
  :322:15: warning: acquiring mutex 'guard' requires negative 
capability '!guard' [-Wthread-safety-negative]
MutexLocker guard();
^
  3 warnings generated.

https://godbolt.org/z/5zYT55

If this is the patch that causes it, then i think it's an obvious 
false-positive (wasn't this patch supposed to only handle globals?),
if not, the warning's wording is not great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9dcc82f34ea9: Thread safety analysis: Consider global 
variables in scope (authored by aaronpuchert).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,35 @@
 
 }  // end namespace SimpleTest
 
+Mutex globalMutex;
+
+namespace ScopeTest {
+
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+void fq() EXCLUSIVE_LOCKS_REQUIRED(!::globalMutex);
+
+namespace ns {
+  Mutex globalMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+  void fq() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex);
+}
+
+void testGlobals() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex) {
+  f(); // expected-warning {{calling function 'f' requires negative 
capability '!globalMutex'}}
+  fq();// expected-warning {{calling function 'fq' requires negative 
capability '!globalMutex'}}
+  ns::f();
+  ns::fq();
+}
+
+void testNamespaceGlobals() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex) {
+  f();
+  fq();
+  ns::f();  // expected-warning {{calling function 'f' requires negative 
capability '!globalMutex'}}
+  ns::fq(); // expected-warning {{calling function 'fq' requires negative 
capability '!globalMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) 
__attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+__attribute__((requires_capability(!FlightControl))) {
   const char *msg;
 
   while ((msg = deque_log_msg())) {
@@ -5044,13 +5045,13 @@
   }
 }
 
-void spawn_fake_logger_thread(void) {
+void spawn_fake_logger_thread(void) 
__attribute__((requires_capability(!FlightControl))) {
   acquire(Logger);
   logger_entry();
   release(Logger);
 }
 
-int main(void) {
+int main(void) __attribute__((requires_capability(!FlightControl))) {
   spawn_fake_flight_control_thread();
   spawn_fake_logger_thread();
 
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1266,13 +1266,22 @@
 }
 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr ) {
-  if (!CurrentMethod)
+  const threadSafety::til::SExpr *SExp = CapE.sexpr();
+  assert(SExp && "Null expressions should be ignored");
+
+  // Global variables are always in scope.
+  if (isa(SExp))
+return true;
+
+  // Members are in scope from methods of the same class.
+  if (const auto *P = dyn_cast(SExp)) {
+if (!CurrentMethod)
   return false;
-  if (const auto *P = dyn_cast_or_null(CapE.sexpr())) {
 const auto *VD = P->clangDecl();
 if (VD)
   return VD->getDeclContext() == CurrentMethod->getDeclContext();
   }
+
   return false;
 }
 


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,35 @@
 
 }  // end namespace SimpleTest
 
+Mutex globalMutex;
+
+namespace ScopeTest {
+
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+void fq() EXCLUSIVE_LOCKS_REQUIRED(!::globalMutex);
+
+namespace ns {
+  Mutex globalMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+  void fq() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex);
+}
+
+void testGlobals() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex) {
+  f(); // expected-warning {{calling function 'f' requires negative capability '!globalMutex'}}
+  fq();// expected-warning {{calling function 'fq' requires negative capability '!globalMutex'}}
+  ns::f();
+  ns::fq();
+}
+
+void testNamespaceGlobals() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex) {
+  f();
+  fq();
+  ns::f();  // expected-warning {{calling function 'f' requires negative capability '!globalMutex'}}
+  ns::fq(); // expected-warning {{calling function 'fq' requires negative capability '!globalMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 289833.
aaronpuchert marked 2 inline comments as done.
aaronpuchert added a comment.

Add tests with qualified names, let tests rely on shadowing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,35 @@
 
 }  // end namespace SimpleTest
 
+Mutex globalMutex;
+
+namespace ScopeTest {
+
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+void fq() EXCLUSIVE_LOCKS_REQUIRED(!::globalMutex);
+
+namespace ns {
+  Mutex globalMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+  void fq() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex);
+}
+
+void testGlobals() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex) {
+  f(); // expected-warning {{calling function 'f' requires negative 
capability '!globalMutex'}}
+  fq();// expected-warning {{calling function 'fq' requires negative 
capability '!globalMutex'}}
+  ns::f();
+  ns::fq();
+}
+
+void testNamespaceGlobals() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex) {
+  f();
+  fq();
+  ns::f();  // expected-warning {{calling function 'f' requires negative 
capability '!globalMutex'}}
+  ns::fq(); // expected-warning {{calling function 'fq' requires negative 
capability '!globalMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) 
__attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+__attribute__((requires_capability(!FlightControl))) {
   const char *msg;
 
   while ((msg = deque_log_msg())) {
@@ -5044,13 +5045,13 @@
   }
 }
 
-void spawn_fake_logger_thread(void) {
+void spawn_fake_logger_thread(void) 
__attribute__((requires_capability(!FlightControl))) {
   acquire(Logger);
   logger_entry();
   release(Logger);
 }
 
-int main(void) {
+int main(void) __attribute__((requires_capability(!FlightControl))) {
   spawn_fake_flight_control_thread();
   spawn_fake_logger_thread();
 
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1266,13 +1266,22 @@
 }
 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr ) {
-  if (!CurrentMethod)
+  const threadSafety::til::SExpr *SExp = CapE.sexpr();
+  assert(SExp && "Null expressions should be ignored");
+
+  // Global variables are always in scope.
+  if (isa(SExp))
+return true;
+
+  // Members are in scope from methods of the same class.
+  if (const auto *P = dyn_cast(SExp)) {
+if (!CurrentMethod)
   return false;
-  if (const auto *P = dyn_cast_or_null(CapE.sexpr())) {
 const auto *VD = P->clangDecl();
 if (VD)
   return VD->getDeclContext() == CurrentMethod->getDeclContext();
   }
+
   return false;
 }
 


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,35 @@
 
 }  // end namespace SimpleTest
 
+Mutex globalMutex;
+
+namespace ScopeTest {
+
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+void fq() EXCLUSIVE_LOCKS_REQUIRED(!::globalMutex);
+
+namespace ns {
+  Mutex globalMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+  void fq() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex);
+}
+
+void testGlobals() EXCLUSIVE_LOCKS_REQUIRED(!ns::globalMutex) {
+  f(); // expected-warning {{calling function 'f' requires negative capability '!globalMutex'}}
+  fq();// expected-warning {{calling function 'fq' requires negative capability '!globalMutex'}}
+  ns::f();
+  ns::fq();
+}
+
+void testNamespaceGlobals() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex) {
+  f();
+  fq();
+  ns::f();  // expected-warning {{calling function 'f' requires negative capability '!globalMutex'}}
+  ns::fq(); // expected-warning {{calling function 'fq' requires negative capability '!globalMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87
+Mutex globalMutex;
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+

aaron.ballman wrote:
> Can you add a test that uses `!::globalMutex`? I'd like to verify that lookup 
> rules are honored for naming the mutex, so more complex examples with name 
> hiding would also be useful.
Just noticed that we're in namespace `ScopeTest` here, I'll probably need to 
move that mutex out first. For name hiding I can just name both mutexes the 
same, I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:87
+Mutex globalMutex;
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+

Can you add a test that uses `!::globalMutex`? I'd like to verify that lookup 
rules are honored for naming the mutex, so more complex examples with name 
hiding would also be useful.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:91
+  Mutex namespaceMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!namespaceMutex);
+}

Can you also add a test that uses `!ns::namespaceMutex`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

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

Rebase on top of D84603 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,23 @@
 
 }  // end namespace SimpleTest
 
+namespace ScopeTest {
+
+Mutex globalMutex;
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+
+namespace ns {
+  Mutex namespaceMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!namespaceMutex);
+}
+
+void testGlobals() {
+  f(); // expected-warning {{calling function 'f' requires negative 
capability '!globalMutex'}}
+  ns::f(); // expected-warning {{calling function 'f' requires negative 
capability '!namespaceMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) 
__attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+__attribute__((requires_capability(!FlightControl))) {
   const char *msg;
 
   while ((msg = deque_log_msg())) {
@@ -5044,13 +5045,13 @@
   }
 }
 
-void spawn_fake_logger_thread(void) {
+void spawn_fake_logger_thread(void) 
__attribute__((requires_capability(!FlightControl))) {
   acquire(Logger);
   logger_entry();
   release(Logger);
 }
 
-int main(void) {
+int main(void) __attribute__((requires_capability(!FlightControl))) {
   spawn_fake_flight_control_thread();
   spawn_fake_logger_thread();
 
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1266,13 +1266,22 @@
 }
 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr ) {
-  if (!CurrentMethod)
+  const threadSafety::til::SExpr *SExp = CapE.sexpr();
+  assert(SExp && "Null expressions should be ignored");
+
+  // Global variables are always in scope.
+  if (isa(SExp))
+return true;
+
+  // Members are in scope from methods of the same class.
+  if (const auto *P = dyn_cast(SExp)) {
+if (!CurrentMethod)
   return false;
-  if (const auto *P = dyn_cast_or_null(CapE.sexpr())) {
 const auto *VD = P->clangDecl();
 if (VD)
   return VD->getDeclContext() == CurrentMethod->getDeclContext();
   }
+
   return false;
 }
 


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,23 @@
 
 }  // end namespace SimpleTest
 
+namespace ScopeTest {
+
+Mutex globalMutex;
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+
+namespace ns {
+  Mutex namespaceMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!namespaceMutex);
+}
+
+void testGlobals() {
+  f(); // expected-warning {{calling function 'f' requires negative capability '!globalMutex'}}
+  ns::f(); // expected-warning {{calling function 'f' requires negative capability '!namespaceMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) __attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+__attribute__((requires_capability(!FlightControl))) {
   const char *msg;
 
   while ((msg = deque_log_msg())) {
@@ -5044,13 +5045,13 @@
   }
 }
 
-void spawn_fake_logger_thread(void) {
+void spawn_fake_logger_thread(void) __attribute__((requires_capability(!FlightControl))) {
   acquire(Logger);
   logger_entry();
   release(Logger);
 }
 
-int main(void) {
+int main(void) __attribute__((requires_capability(!FlightControl))) {
   spawn_fake_flight_control_thread();
   spawn_fake_logger_thread();
 
Index: clang/lib/Analysis/ThreadSafety.cpp

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Test failure is expected because I based this on D84603 
 locally, but I'm not sure how to tell that to 
Phabricator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604



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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a subscriber: anarazel.
aaronpuchert added a comment.

@anarazel, that should fix the issue you reported on IRC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604



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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

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

Instead of just mutex members we also consider mutex globals.
Unsurprisingly they are always in scope. Now the paper says that

  The scope of a class member is assumed to be its enclosing class,
  while the scope of a global variable is the translation unit in
  which it is defined.

But I don't think we should limit this to TUs where a definition is
available - a declaration is enough to acquire the mutex, and if a mutex
is really limited in scope to a translation unit, it should probably be
only declared there.

Fixes PR46354.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84604

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


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,23 @@
 
 }  // end namespace SimpleTest
 
+namespace ScopeTest {
+
+Mutex globalMutex;
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+
+namespace ns {
+  Mutex namespaceMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!namespaceMutex);
+}
+
+void testGlobals() {
+  f(); // expected-warning {{calling function 'f' requires holding 
negative capability '!globalMutex'}}
+  ns::f(); // expected-warning {{calling function 'f' requires holding 
negative capability '!namespaceMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) 
__attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+__attribute__((requires_capability(!FlightControl))) {
   const char *msg;
 
   while ((msg = deque_log_msg())) {
@@ -5044,13 +5045,13 @@
   }
 }
 
-void spawn_fake_logger_thread(void) {
+void spawn_fake_logger_thread(void) 
__attribute__((requires_capability(!FlightControl))) {
   acquire(Logger);
   logger_entry();
   release(Logger);
 }
 
-int main(void) {
+int main(void) __attribute__((requires_capability(!FlightControl))) {
   spawn_fake_flight_control_thread();
   spawn_fake_logger_thread();
 
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1266,13 +1266,22 @@
 }
 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr ) {
-  if (!CurrentMethod)
+  const threadSafety::til::SExpr *SExp = CapE.sexpr();
+  assert(SExp && "Null expressions should be ignored");
+
+  // Global variables are always in scope.
+  if (isa(SExp))
+return true;
+
+  // Members are in scope from methods of the same class.
+  if (const auto *P = dyn_cast(SExp)) {
+if (!CurrentMethod)
   return false;
-  if (const auto *P = dyn_cast_or_null(CapE.sexpr())) {
 const auto *VD = P->clangDecl();
 if (VD)
   return VD->getDeclContext() == CurrentMethod->getDeclContext();
   }
+
   return false;
 }
 


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,23 @@
 
 }  // end namespace SimpleTest
 
+namespace ScopeTest {
+
+Mutex globalMutex;
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+
+namespace ns {
+  Mutex namespaceMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!namespaceMutex);
+}
+
+void testGlobals() {
+  f(); // expected-warning {{calling function 'f' requires holding negative capability '!globalMutex'}}
+  ns::f(); // expected-warning {{calling function 'f' requires holding negative capability '!namespaceMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) __attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+