[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-11-03 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert planned changes to this revision.
aaronpuchert added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87194/new/

https://reviews.llvm.org/D87194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-11-02 Thread Jordan Rupprecht via Phabricator via cfe-commits 
rupprecht added a comment.

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 ¶ms;
  }

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87194/new/

https://reviews.llvm.org/D87194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-11-02 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added a subscriber: rupprecht.
aaronpuchert added a comment.

@rupprecht, maybe you can try it again?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87194/new/

https://reviews.llvm.org/D87194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-11-02 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert updated this revision to Diff 302388.
aaronpuchert marked 2 inline comments as done.
aaronpuchert added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87194/new/

https://reviews.llvm.org/D87194

Files:
  clang/lib/Analysis/ThreadSafety.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
@@ -124,9 +124,9 @@
   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;
@@ -140,7 +140,54 @@
 
 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();
 }
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -16,6 +16,7 @@
 
 #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"
@@ -1265,6 +1266,35 @@
   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(VD->getDeclContext()),
+ *MethodRecord =
+ cast(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");
@@ -1274,20 +1304,14 @@
 // 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(VD->getDeclContext()))
-  return false;
+  return accessibleMember(VD, CurrentMethod);
 // Global variables are always in scope.
 retur

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-11-02 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added inline comments.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:158
+}
+
 }  // end namespace ScopeTest

aaron.ballman wrote:
> 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)
Isn't there a semantic and a lexical declaration context, and aren't we 
consistently using the semantic context?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87194/new/

https://reviews.llvm.org/D87194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-11-02 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/lib/Analysis/ThreadSafety.cpp:1283
+  return true;
+// We are ignoring friends here.
 if (!CurrentMethod)

This should probably have a FIXME?



Comment at: clang/lib/Analysis/ThreadSafety.cpp:1297-1299
+  for (const CXXBasePath &Path : Paths)
+if (Path.Access != AS_none)
+  return true;

`llvm::any_of()`?



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:158
+}
+
 }  // end namespace ScopeTest

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87194/new/

https://reviews.llvm.org/D87194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-10-30 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert planned changes to this revision.
aaronpuchert added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87194/new/

https://reviews.llvm.org/D87194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

2020-09-05 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert created this revision.
aaronpuchert added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
aaronpuchert requested review of this revision.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87194

Files:
  clang/lib/Analysis/ThreadSafety.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
@@ -108,6 +108,54 @@
   ns::fq(); // expected-warning {{calling function 'fq' requires negative capability '!globalMutex'}}
 }
 
+class Base {
+public:
+  void nopub() EXCLUSIVE_LOCKS_REQUIRED(!pub);
+  void noprot() EXCLUSIVE_LOCKS_REQUIRED(!prot);
+  void nopriv() EXCLUSIVE_LOCKS_REQUIRED(!priv);
+
+  void inClass() {
+nopub();  // expected-warning {{calling function 'nopub' requires negative capability '!pub'}}
+noprot(); // expected-warning {{calling function 'noprot' requires negative capability '!prot'}}
+nopriv(); // expected-warning {{calling function 'nopriv' requires negative capability '!priv'}}
+  }
+
+  Mutex pub;
+
+protected:
+  Mutex prot;
+
+private:
+  Mutex priv;
+};
+
+class Derived : private Base {
+public:
+  void nopubDerived() EXCLUSIVE_LOCKS_REQUIRED(!pub);
+  void noprotDerived() EXCLUSIVE_LOCKS_REQUIRED(!prot);
+
+  void inDerivedClass() {
+nopub();  // expected-warning {{calling function 'nopub' requires negative capability '!pub'}}
+noprot(); // expected-warning {{calling function 'noprot' requires negative capability '!prot'}}
+nopriv();
+  }
+};
+
+class MoreDerived : public Derived {
+public:
+  void inDerivedClassWithInacessibleBase() {
+nopubDerived(); // expected-warning {{calling function 'nopubDerived' requires negative capability '!pub'}}
+noprotDerived();
+  }
+};
+
+void outside() {
+  Base x;
+  x.nopub(); // expected-warning {{calling function 'nopub' requires negative capability '!x.pub'}}
+  x.noprot();
+  x.nopriv();
+}
+
 }  // end namespace ScopeTest
 
 namespace DoubleAttribute {
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -16,6 +16,7 @@
 
 #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"
@@ -1275,10 +1276,28 @@
 
   // Members are in scope from methods of the same class.
   if (const auto *P = dyn_cast(SExp)) {
+const ValueDecl *VD = P->clangDecl();
+// Public members are always accessible.
+if (VD->getAccess() == AS_public)
+  return true;
+// We are ignoring friends here.
 if (!CurrentMethod)
   return false;
-const ValueDecl *VD = P->clangDecl();
-return VD->getDeclContext() == CurrentMethod->getDeclContext();
+const auto *CapRecord = cast(VD->getDeclContext()),
+   *MethodRecord =
+   cast(CurrentMethod->getDeclContext());
+// If we're in the same record, all members are accessible.
+if (CapRecord == MethodRecord)
+  return true;
+// Additionally in derived classes, protected members are accessible.
+if (VD->getAccess() == AS_protected) {
+  CXXBasePaths Paths;
+  if (!MethodRecord->isDerivedFrom(CapRecord, Paths))
+return false;
+  for (const CXXBasePath &Path : Paths)
+if (Path.Access != AS_none)
+  return true;
+}
   }
 
   return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits