Thread safety analysis: Use access specifiers to decide about scope

[aaronpuchert]

Summary

Public members are always in scope, protected members in derived classes
with sufficient access.

[aaronpuchert]

Need to rebase this on top of D84604 plus a subsequent fix.

[aaron.ballman]

clang/lib/Analysis/ThreadSafety.cpp

1317

This should probably have a FIXME?

1323–1325

llvm::any_of()?

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

168

Given that this is touching on declaration contexts, it would also be good to have some tests checking for differences between declaration contexts (like out-of-line method definitions)

[aaronpuchert]

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

168

Isn't there a semantic and a lexical declaration context, and aren't we consistently using the semantic context?

[aaronpuchert]

Rebased on D84604, included static members, and addressed review comments.

[aaronpuchert]

@rupprecht, maybe you can try it again?

[rupprecht]

In D87194#2369485, @aaronpuchert wrote:

@rupprecht, maybe you can try it again?

Some more interesting errors this time :)

The ones I originally saw look correct now (i.e. it's flagging the things that are valid, but not the things out of visibility). I tried building the rest of this package, and I guess scoping isn't considered in this case though?

class Foo {
 public:
  static void Bar();
 private:
  struct Params {
    Mutex mu_;
  }
  static Params* GetParams();
};

// In the .cc file:
void Foo::Bar() {
  Params& params = *GetParams();
  MutexLock lock(params.mu_);  // error: acquiring mutex 'params.mu_' requires negative capability '!params.mu_'
}

/* static */ Params* Foo::GetParams() {
  static Params params;
  return &params;
}

On one hand, it's totally valid. On the other hand, annotating the method like static void Bar() REQUIRES(!params.mu_); isn't possible because params is a local variable.

(I'm new to threading analysis, so maybe I'm just using the wrong annotations)

[aaronpuchert]

That's a good point, while mu_ is public, params is a local variable.

I need to take into account the left-hand side of a til::Project, which we're currently ignoring.

clang/lib/Analysis/ThreadSafety.cpp

 //
 // See http://clang.llvm.org/docs/ThreadSafetyAnalysis.html
 // for more information.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/Analyses/ThreadSafety.h"
 #include "clang/AST/Attr.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclGroup.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/OperationKinds.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtVisitor.h"

 ClassifyDiagnostic(const AttrTy *A) {
   for (const auto *Arg : A->args()) {
     if (const ValueDecl *VD = getValueDecl(Arg))
       return ClassifyDiagnostic(VD);
   }
   return "mutex";
 }
 
+static bool accessibleMember(const ValueDecl *VD,
+                             const CXXMethodDecl *CurrentMethod) {
+  AccessSpecifier Access = VD->getAccess();
+  if (Access == AS_public)
+    return true;
+
+  // FIXME: We are ignoring friends for now.
+  if (!CurrentMethod)
+    return false;
+
+  const auto *ValueDeclRecord = cast<CXXRecordDecl>(VD->getDeclContext()),
+             *MethodRecord =
+                 cast<CXXRecordDecl>(CurrentMethod->getDeclContext());
+
+  // If we're in the same class, all members are accessible.
+  if (ValueDeclRecord == MethodRecord)
+    return true;
+  // Otherwise we're out of luck for private members.
+  if (Access == AS_private)
+    return false;
+  // Protected members might be accessible in derived classes.
+  assert(Access == AS_protected);
+  CXXBasePaths Paths;
+  return MethodRecord->isDerivedFrom(ValueDeclRecord, Paths) &&
+         llvm::any_of(Paths, [](const CXXBasePath &Path) {
+           return Path.Access != AS_none;
+         });
+}
+
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) {
   const threadSafety::til::SExpr *SExp = CapE.sexpr();
   assert(SExp && "Null expressions should be ignored");
 
   if (const auto *LP = dyn_cast<til::LiteralPtr>(SExp)) {
     const ValueDecl *VD = LP->clangDecl();
     // Variables defined in a function are always inaccessible.
     if (!VD->isDefinedOutsideFunctionOrMethod())
       return false;
-    // For now we consider static class members to be inaccessible.
     if (isa<CXXRecordDecl>(VD->getDeclContext()))
-      return false;
+      return accessibleMember(VD, CurrentMethod);
     // Global variables are always in scope.
     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;
-    const ValueDecl *VD = P->clangDecl();
-    return VD->getDeclContext() == CurrentMethod->getDeclContext();
-  }
+  if (const auto *P = dyn_cast<til::Project>(SExp))
+    return accessibleMember(P->clangDecl(), CurrentMethod);
 
   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) {
   if (Entry->shouldIgnore())
     return;
 
   if (!ReqAttr && !Entry->negative()) {
     // look for the negative capability, and remove it from the fact set.
     CapabilityExpr NegC = !*Entry;
     const FactEntry *Nen = FSet.findLock(FactMan, NegC);
     if (Nen) {
       FSet.removeLock(FactMan, NegC);
     }

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

 }
 
 class StaticMembers {
 public:
   void pub() EXCLUSIVE_LOCKS_REQUIRED(!publicMutex);
   void prot() EXCLUSIVE_LOCKS_REQUIRED(!protectedMutex);
   void priv() EXCLUSIVE_LOCKS_REQUIRED(!privateMutex);
   void test() {
-    pub();
-    prot();
-    priv();
+    pub();  // expected-warning {{calling function 'pub' requires negative capability '!publicMutex'}}
+    prot(); // expected-warning {{calling function 'prot' requires negative capability '!protectedMutex'}}
+    priv(); // expected-warning {{calling function 'priv' requires negative capability '!privateMutex'}}
   }
 
   static Mutex publicMutex;
 
 protected:
   static Mutex protectedMutex;
 
 private:
   static Mutex privateMutex;
 };
 
 void testStaticMembers() {
   StaticMembers x;
-  x.pub();
+  x.pub(); // expected-warning {{calling function 'pub' requires negative capability '!publicMutex'}}
+  x.prot();
+  x.priv();
+}
+
+class Base {
+public:
+  void pub() EXCLUSIVE_LOCKS_REQUIRED(!publicMutex);
+  void prot() EXCLUSIVE_LOCKS_REQUIRED(!protectedMutex);
+  void priv() EXCLUSIVE_LOCKS_REQUIRED(!privateMutex);
+  void test() {
+    pub();  // expected-warning {{calling function 'pub' requires negative capability '!publicMutex'}}
+    prot(); // expected-warning {{calling function 'prot' requires negative capability '!protectedMutex'}}
+    priv(); // expected-warning {{calling function 'priv' requires negative capability '!privateMutex'}}
+  }
+
+  static Mutex publicMutex;
+
+protected:
+  static Mutex protectedMutex;
+
+private:
+  static Mutex privateMutex;
+};
+
+class Derived : private Base {
+public:
+  void pubDerived() EXCLUSIVE_LOCKS_REQUIRED(!publicMutex);
+  void protDerived() EXCLUSIVE_LOCKS_REQUIRED(!protectedMutex);
+
+  void inDerivedClass() {
+    pub();  // expected-warning {{calling function 'pub' requires negative capability '!publicMutex'}}
+    prot(); // expected-warning {{calling function 'prot' requires negative capability '!protectedMutex'}}
+    priv();
+  }
+};
+
+class MoreDerived : public Derived {
+public:
+  void inDerivedClassWithInacessibleBase() {
+    pubDerived(); // expected-warning {{calling function 'pubDerived' requires negative capability '!publicMutex'}}
+    protDerived();
+  }
+};
+
+void outside() {
+  Base x;
+  x.pub(); // expected-warning {{calling function 'pub' requires negative capability '!publicMutex'}}
   x.prot();
   x.priv();
 }
 
 }  // end namespace ScopeTest
 
 namespace DoubleAttribute {