[PATCH] D72635: Add "context" capability to Thread Safety Analysis

[Etienne Pierre-Doray via Phabricator via cfe-commits]

eti-p-doray created this revision.
eti-p-doray added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Main use case: 
Chromium has runtime "SequenceChecker" and "ThreadChecker" that enforce 
sequenced task context. Thread safety analysis can be used but neither "mutex" 
nor "role" seem appropriate for meaningful error messages.
https://chromium-review.googlesource.com/c/chromium/src/+/1948098


Repository:
  rC Clang

https://reviews.llvm.org/D72635

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp


Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6199,10 +6199,12 @@
   !S.checkStringLiteralArgumentAttr(AL, 0, N, &LiteralLoc))
 return;
 
-  // Currently, there are only two names allowed for a capability: role and
-  // mutex (case insensitive). Diagnose other capability names.
-  if (!N.equals_lower("mutex") && !N.equals_lower("role"))
+  // Currently, there are only 3 names allowed for a capability: role,
+  // mutex and context (case insensitive). Diagnose other capability names.
+  if (!N.equals_lower("mutex") && !N.equals_lower("role") &&
+  !N.equals_lower("context")) {
 S.Diag(LiteralLoc, diag::warn_invalid_capability_name) << N;
+  }
 
   D->addAttr(::new (S.Context) CapabilityAttr(S.Context, AL, N));
 }
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3196,7 +3196,8 @@
 
 // Thread Safety Attributes
 def warn_invalid_capability_name : Warning<
-  "invalid capability name '%0'; capability name must be 'mutex' or 'role'">,
+  "invalid capability name '%0'; capability name must be 'mutex', 'role' or "
+  "'context'">,
   InGroup, DefaultIgnore;
 def warn_thread_attribute_ignored : Warning<
   "ignoring %0 attribute because its argument is invalid">,
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2565,6 +2565,7 @@
   let AdditionalMembers = [{
 bool isMutex() const { return getName().equals_lower("mutex"); }
 bool isRole() const { return getName().equals_lower("role"); }
+bool isContext() const { return getName().equals_lower("context"); }
   }];
 }
 


Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6199,10 +6199,12 @@
   !S.checkStringLiteralArgumentAttr(AL, 0, N, &LiteralLoc))
 return;
 
-  // Currently, there are only two names allowed for a capability: role and
-  // mutex (case insensitive). Diagnose other capability names.
-  if (!N.equals_lower("mutex") && !N.equals_lower("role"))
+  // Currently, there are only 3 names allowed for a capability: role,
+  // mutex and context (case insensitive). Diagnose other capability names.
+  if (!N.equals_lower("mutex") && !N.equals_lower("role") &&
+  !N.equals_lower("context")) {
 S.Diag(LiteralLoc, diag::warn_invalid_capability_name) << N;
+  }
 
   D->addAttr(::new (S.Context) CapabilityAttr(S.Context, AL, N));
 }
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3196,7 +3196,8 @@
 
 // Thread Safety Attributes
 def warn_invalid_capability_name : Warning<
-  "invalid capability name '%0'; capability name must be 'mutex' or 'role'">,
+  "invalid capability name '%0'; capability name must be 'mutex', 'role' or "
+  "'context'">,
   InGroup, DefaultIgnore;
 def warn_thread_attribute_ignored : Warning<
   "ignoring %0 attribute because its argument is invalid">,
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2565,6 +2565,7 @@
   let AdditionalMembers = [{
 bool isMutex() const { return getName().equals_lower("mutex"); }
 bool isRole() const { return getName().equals_lower("role"); }
+bool isContext() const { return getName().equals_lower("context"); }
   }];
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72635: Add "context" capability to Thread Safety Analysis

[Etienne Pierre-Doray via Phabricator via cfe-commits]

eti-p-doray added a comment.

I added an example in the description.

From doc https://clang.llvm.org/docs/ThreadSafetyAnalysis.html, 
it sounds like we should be allowed to declare our class with 
CAPABILITY("context"), but it turns out that only "mutex" and "role" are 
allowed.
I could otherwise update this CL to allow any string (single word lowercase?) 
as CAPABILITY?


Repository:
  rC Clang

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

https://reviews.llvm.org/D72635



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

[PATCH] D72635: Allow arbitrary capability name in Thread Safety Analysis

[Etienne Pierre-Doray via Phabricator via cfe-commits]

eti-p-doray updated this revision to Diff 238782.
eti-p-doray retitled this revision from "Add "context" capability to Thread 
Safety Analysis" to "Allow arbitrary capability name in Thread Safety Analysis".
eti-p-doray added a comment.

Allow arbitrary capability


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

https://reviews.llvm.org/D72635

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-capabilities.c


Index: clang/test/Sema/attr-capabilities.c
===
--- clang/test/Sema/attr-capabilities.c
+++ clang/test/Sema/attr-capabilities.c
@@ -8,9 +8,6 @@
 union __attribute__((capability("mutex"))) MutexUnion { int a; char* b; };
 typedef union { int a; char* b; } __attribute__((capability("mutex"))) 
MutexUnion2;
 
-// Test an invalid capability name
-struct __attribute__((capability("wrong"))) IncorrectName {}; // 
expected-warning {{invalid capability name 'wrong'; capability name must be 
'mutex' or 'role'}}
-
 int Test1 __attribute__((capability("test1")));  // expected-error 
{{'capability' attribute only applies to structs, unions, classes, and 
typedefs}}
 int Test2 __attribute__((shared_capability("test2"))); // expected-error 
{{'shared_capability' attribute only applies to structs, unions, classes, and 
typedefs}}
 int Test3 __attribute__((acquire_capability("test3")));  // expected-warning 
{{'acquire_capability' attribute only applies to functions}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6199,11 +6199,6 @@
   !S.checkStringLiteralArgumentAttr(AL, 0, N, &LiteralLoc))
 return;
 
-  // Currently, there are only two names allowed for a capability: role and
-  // mutex (case insensitive). Diagnose other capability names.
-  if (!N.equals_lower("mutex") && !N.equals_lower("role"))
-S.Diag(LiteralLoc, diag::warn_invalid_capability_name) << N;
-
   D->addAttr(::new (S.Context) CapabilityAttr(S.Context, AL, N));
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3195,9 +3195,6 @@
   InGroup>;
 
 // Thread Safety Attributes
-def warn_invalid_capability_name : Warning<
-  "invalid capability name '%0'; capability name must be 'mutex' or 'role'">,
-  InGroup, DefaultIgnore;
 def warn_thread_attribute_ignored : Warning<
   "ignoring %0 attribute because its argument is invalid">,
   InGroup, DefaultIgnore;
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2562,10 +2562,6 @@
   let Accessors = [Accessor<"isShared",
 [Clang<"shared_capability", 0>]>];
   let Documentation = [Undocumented];
-  let AdditionalMembers = [{
-bool isMutex() const { return getName().equals_lower("mutex"); }
-bool isRole() const { return getName().equals_lower("role"); }
-  }];
 }
 
 def AssertCapability : InheritableAttr {


Index: clang/test/Sema/attr-capabilities.c
===
--- clang/test/Sema/attr-capabilities.c
+++ clang/test/Sema/attr-capabilities.c
@@ -8,9 +8,6 @@
 union __attribute__((capability("mutex"))) MutexUnion { int a; char* b; };
 typedef union { int a; char* b; } __attribute__((capability("mutex"))) MutexUnion2;
 
-// Test an invalid capability name
-struct __attribute__((capability("wrong"))) IncorrectName {}; // expected-warning {{invalid capability name 'wrong'; capability name must be 'mutex' or 'role'}}
-
 int Test1 __attribute__((capability("test1")));  // expected-error {{'capability' attribute only applies to structs, unions, classes, and typedefs}}
 int Test2 __attribute__((shared_capability("test2"))); // expected-error {{'shared_capability' attribute only applies to structs, unions, classes, and typedefs}}
 int Test3 __attribute__((acquire_capability("test3")));  // expected-warning {{'acquire_capability' attribute only applies to functions}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6199,11 +6199,6 @@
   !S.checkStringLiteralArgumentAttr(AL, 0, N, &LiteralLoc))
 return;
 
-  // Currently, there are only two names allowed for a capability: role and
-  // mutex (case insensitive). Diagnose other capability names.
-  if (!N.equals_lower("mutex") && !N.equals_lower("role"))
-S.Diag(LiteralLoc, diag::warn_invalid_capability_name) << N;
-
   D->addAttr(::new (S.Context) CapabilityAttr(S.Context, AL, N));
 }
 
Index: clang/inc

[PATCH] D72635: Allow arbitrary capability name in Thread Safety Analysis

[Etienne Pierre-Doray via Phabricator via cfe-commits]

eti-p-doray added a comment.

Thank you for checking! I updated the changes to lift the restriction. PTAnL?


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

https://reviews.llvm.org/D72635



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

[PATCH] D72635: Allow arbitrary capability name in Thread Safety Analysis

[Etienne Pierre-Doray via Phabricator via cfe-commits]

eti-p-doray updated this revision to Diff 239362.
eti-p-doray added a comment.

Added test with custom capability name.


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

https://reviews.llvm.org/D72635

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-capabilities.c


Index: clang/test/Sema/attr-capabilities.c
===
--- clang/test/Sema/attr-capabilities.c
+++ clang/test/Sema/attr-capabilities.c
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -Wthread-safety -verify %s
 
+typedef int __attribute__((capability("role"))) ThreadRole;
 typedef int __attribute__((capability("role"))) ThreadRole;
 struct __attribute__((shared_capability("mutex"))) Mutex {};
 struct NotACapability {};
@@ -8,8 +9,8 @@
 union __attribute__((capability("mutex"))) MutexUnion { int a; char* b; };
 typedef union { int a; char* b; } __attribute__((capability("mutex"))) 
MutexUnion2;
 
-// Test an invalid capability name
-struct __attribute__((capability("wrong"))) IncorrectName {}; // 
expected-warning {{invalid capability name 'wrong'; capability name must be 
'mutex' or 'role'}}
+// Test a different capability name
+struct __attribute__((capability("custom"))) CustomName {};
 
 int Test1 __attribute__((capability("test1")));  // expected-error 
{{'capability' attribute only applies to structs, unions, classes, and 
typedefs}}
 int Test2 __attribute__((shared_capability("test2"))); // expected-error 
{{'shared_capability' attribute only applies to structs, unions, classes, and 
typedefs}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6199,11 +6199,6 @@
   !S.checkStringLiteralArgumentAttr(AL, 0, N, &LiteralLoc))
 return;
 
-  // Currently, there are only two names allowed for a capability: role and
-  // mutex (case insensitive). Diagnose other capability names.
-  if (!N.equals_lower("mutex") && !N.equals_lower("role"))
-S.Diag(LiteralLoc, diag::warn_invalid_capability_name) << N;
-
   D->addAttr(::new (S.Context) CapabilityAttr(S.Context, AL, N));
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3195,9 +3195,6 @@
   InGroup>;
 
 // Thread Safety Attributes
-def warn_invalid_capability_name : Warning<
-  "invalid capability name '%0'; capability name must be 'mutex' or 'role'">,
-  InGroup, DefaultIgnore;
 def warn_thread_attribute_ignored : Warning<
   "ignoring %0 attribute because its argument is invalid">,
   InGroup, DefaultIgnore;
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2562,10 +2562,6 @@
   let Accessors = [Accessor<"isShared",
 [Clang<"shared_capability", 0>]>];
   let Documentation = [Undocumented];
-  let AdditionalMembers = [{
-bool isMutex() const { return getName().equals_lower("mutex"); }
-bool isRole() const { return getName().equals_lower("role"); }
-  }];
 }
 
 def AssertCapability : InheritableAttr {


Index: clang/test/Sema/attr-capabilities.c
===
--- clang/test/Sema/attr-capabilities.c
+++ clang/test/Sema/attr-capabilities.c
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -Wthread-safety -verify %s
 
+typedef int __attribute__((capability("role"))) ThreadRole;
 typedef int __attribute__((capability("role"))) ThreadRole;
 struct __attribute__((shared_capability("mutex"))) Mutex {};
 struct NotACapability {};
@@ -8,8 +9,8 @@
 union __attribute__((capability("mutex"))) MutexUnion { int a; char* b; };
 typedef union { int a; char* b; } __attribute__((capability("mutex"))) MutexUnion2;
 
-// Test an invalid capability name
-struct __attribute__((capability("wrong"))) IncorrectName {}; // expected-warning {{invalid capability name 'wrong'; capability name must be 'mutex' or 'role'}}
+// Test a different capability name
+struct __attribute__((capability("custom"))) CustomName {};
 
 int Test1 __attribute__((capability("test1")));  // expected-error {{'capability' attribute only applies to structs, unions, classes, and typedefs}}
 int Test2 __attribute__((shared_capability("test2"))); // expected-error {{'shared_capability' attribute only applies to structs, unions, classes, and typedefs}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6199,11 +6199,6 @@
   !S.checkStringLiteralArgumentAttr(AL, 0, N, &LiteralLoc))
 return;
 
-  // 

[PATCH] D72635: Allow arbitrary capability name in Thread Safety Analysis

[Etienne Pierre-Doray via Phabricator via cfe-commits]

eti-p-doray added a comment.

Thanks!
I don't I have commit access. Can you commit on my behalf?


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

https://reviews.llvm.org/D72635



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