[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 &CapE) {
-  if (!CurrentMethod)
+  const threadSafety::til::SExpr *SExp = CapE.sexpr();
+  assert(SExp && "Null expressions should be ignored");
+
+  // Global variables are always in scope.
+  if (isa<til::LiteralPtr>(SExp))
+    return true;
+
+  // 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;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D84604.280742.patch
Type: text/x-patch
Size: 2747 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200726/d71a0d32/attachment.bin>