Thread safety analysis: Consider global variables in scope

[aaronpuchert]

Summary

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.

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

Fixes PR46354.

[aaronpuchert]

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

[aaronpuchert]

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

[aaronpuchert]

Rebase on top of D84603.

[aaron.ballman]

clang/test/SemaCXX/warn-thread-safety-negative.cpp

87

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.

91

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

[aaronpuchert]

clang/test/SemaCXX/warn-thread-safety-negative.cpp

87

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.

[aaronpuchert]

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

[aaron.ballman]

LGTM, thanks!

[lebedev.ri]

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

<source>:304:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
<source>:309:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
<source>:322:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
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.

[lebedev.ri]

In D84604#2262745, @lebedev.ri wrote:

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

<source>:304:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
<source>:309:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
<source>:322:15: warning: acquiring mutex 'guard' requires negative capability '!guard' [-Wthread-safety-negative]
  MutexLocker guard(&mutex);
              ^
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 rG8427885e27813c457dccb011f65e8ded74444e31.

[aaronpuchert]

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

[aaronpuchert]

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

[aaron.ballman]

LGTM

clang/lib/Analysis/ThreadSafety.cpp

1275

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.

[lebedev.ri]

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

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

1275

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.

clang/test/SemaCXX/warn-thread-safety-negative.cpp

81–83

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

[lebedev.ri]

clang/test/SemaCXX/warn-thread-safety-negative.cpp

81–83

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?

[aaronpuchert]

clang/test/SemaCXX/warn-thread-safety-negative.cpp

81–83

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.

[lebedev.ri]

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

clang/test/SemaCXX/warn-thread-safety-negative.cpp

81–83

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.

[rupprecht]

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

[aaronpuchert]

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.

[aaronpuchert]

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.

[rupprecht]

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.

[aaronpuchert]

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.

[rupprecht]

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.

clang/lib/Analysis/ThreadSafety.cpp

ClassifyDiagnostic(const AttrTy *A) {
   for (const auto *Arg : A->args()) {
     if (const ValueDecl *VD = getValueDecl(Arg))
       return ClassifyDiagnostic(VD);
   }
   return "mutex";
 }
 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) {
+  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;
 }
 
 /// Add a new lock to the lockset, warning if the lock is already there.
 /// \param ReqAttr -- true if this is part of an initial Requires attribute.
 void ThreadSafetyAnalyzer::addLock(FactSet &FSet,
                                    std::unique_ptr<FactEntry> Entry,
                                    StringRef DiagKind, bool ReqAttr) {

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

 
 void spawn_fake_flight_control_thread(void) {
   acquire(FlightControl);
   flight_control_entry();
   release(FlightControl);
 }
 
 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())) {
     dispatch_log(msg);
   }
 }
 
-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();
 
   for (;;)
     ; /* Pretend to dispatch things. */
 
   return 0;
 }

clang/test/SemaCXX/warn-thread-safety-negative.cpp

public:
 
   void test3() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
     mu.Lock();
     a = 0;
     mu.Unlock();
     baz();       // no warning -- !mu in set.
   }
 };
 
 }  // 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 {
   Mutex &mutex();
 };
 
 template <typename A>
 class TemplateClass {
   template <typename B>
   static void Function(Foo *F)
       EXCLUSIVE_LOCKS_REQUIRED(F->mutex()) UNLOCK_FUNCTION(F->mutex()) {}
 };
 
 void test() { TemplateClass<int> TC; }
 
 }  // end namespace DoubleAttribute