Author: Aaron Puchert Date: 2020-10-25T19:32:26+01:00 New Revision: 5250a03a9959c2701a8338fe1a1ffa8bd93d6173
URL: https://github.com/llvm/llvm-project/commit/5250a03a9959c2701a8338fe1a1ffa8bd93d6173 DIFF: https://github.com/llvm/llvm-project/commit/5250a03a9959c2701a8338fe1a1ffa8bd93d6173.diff LOG: 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 Added: Modified: clang/lib/Analysis/ThreadSafety.cpp clang/test/SemaCXX/warn-thread-safety-analysis.cpp clang/test/SemaCXX/warn-thread-safety-negative.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 64e0da9e64b1..ef90fa175a6d 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 91bd15def577..d1520b1decbd 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 456fe16e6574..d7db5f402d11 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 @@ class Foo { 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 { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
