From 5250a03a9959c2701a8338fe1a1ffa8bd93d6173 Mon Sep 17 00:00:00 2001
From: Aaron Puchert <aaronpuchert@alice-dsl.net>
Date: Sun, 25 Oct 2020 19:31:53 +0100
Subject: [PATCH] Thread safety analysis: Consider global variables in scope

Instead of just mutex members we also consider mutex globals.
Unsurprisingly they are always in scope. Now the paper [1] 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.

The previous attempt in 9dcc82f34ea was causing false positives because
I wrongly assumed that LiteralPtrs were always globals, which they are
not. This should be fixed now.

[1] https://static.googleusercontent.com/media/research.google.com/en/us/pubs/archive/42958.pdf

Fixes PR46354.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D84604
---
 clang/lib/Analysis/ThreadSafety.cpp                | 15 +++++++--
 clang/test/SemaCXX/warn-thread-safety-analysis.cpp |  7 ++--
 clang/test/SemaCXX/warn-thread-safety-negative.cpp | 39 ++++++++++++++++++++++
 3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 64e0da9..ef90fa1 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1266,13 +1266,24 @@ ClassifyDiagnostic(const AttrTy *A) {
 }
 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) {
-  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<til::LiteralPtr>(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<til::Project>(SExp)) {
+    if (!CurrentMethod)
       return false;
-  if (const auto *P = dyn_cast_or_null<til::Project>(CapE.sexpr())) {
     const auto *VD = P->clangDecl();
     if (VD)
       return VD->getDeclContext() == CurrentMethod->getDeclContext();
   }
+
   return false;
 }
 
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 91bd15d..d1520b1 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@ void spawn_fake_flight_control_thread(void) {
 }
 
 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 logger_entry(void) __attribute__((requires_capability(Logger))) {
   }
 }
 
-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();
 
diff --git a/clang/test/SemaCXX/warn-thread-safety-negative.cpp b/clang/test/SemaCXX/warn-thread-safety-negative.cpp
index 456fe16..d7db5f4 100644
--- a/clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -21,6 +21,12 @@ class LOCKABLE Mutex {
   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 @@ public:
     mu.Unlock();
     baz();       // no warning -- !mu in set.
   }
+
+  void test4() {
+    MutexLock lock(&mu); // 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 {
-- 
2.7.4