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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D72635#1912501 , @aaron.ballman 
wrote:

> In D72635#1911844 , @aaronpuchert 
> wrote:
>
> > In D72635#1911671 , @aaron.ballman 
> > wrote:
> >
> > > However, do we want to diagnose when the capability strings are different?
> >
> >
> > Now if I acquire capabilities of different types, then the order also has 
> > to incorporate these different types. So my answer would be no, I think we 
> > have to allow this.
>
>
> Okay, I think that makes sense, but is probably something we should mention 
> explicitly in the thread safety documentation so we don't lose track of why 
> this is the way it is. WDYT?


Yes, the documentation could use an overhaul, the behavior of scoped 
capabilities could also be documented better 
.

I have this on my agenda, and I hope I can carve out time for that soon.


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72635#1911844 , @aaronpuchert 
wrote:

> In D72635#1911671 , @aaron.ballman 
> wrote:
>
> > However, do we want to diagnose when the capability strings are different?
>
>
> Hmm, interesting question. The way I think about ordering capabilities is 
> this: assuming I have more than one capability in my program, and 
> occasionally I hold multiple capabilities at the same time. Then deadlocks 
> can occur, unless I'm being careful. One way to avoid deadlocks is to have a 
> partial order on capabilities that is a total order on the subsets of 
> capabilities that are acquired together. It's not hard to see that this is 
> sufficient, and I even think it's necessary. (In the sense that whenever I 
> have a deadlock-free program, such an order exists. I have no proof for that, 
> but that's just for lack of trying.)
>
> Now if I acquire capabilities of different types, then the order also has to 
> incorporate these different types. So my answer would be no, I think we have 
> to allow this.


Okay, I think that makes sense, but is probably something we should mention 
explicitly in the thread safety documentation so we don't lose track of why 
this is the way it is. WDYT?

> In fact (but that's for another change) I've even thought about warning when 
> a cap. is acquired and it's not ordered with another already-held capability 
> (under `-beta` of course).

Ah, interesting idea!


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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D72635#1911671 , @aaron.ballman 
wrote:

> However, do we want to diagnose when the capability strings are different?


Hmm, interesting question. The way I think about ordering capabilities is this: 
assuming I have more than one capability in my program, and occasionally I hold 
multiple capabilities at the same time. Then deadlocks can occur, unless I'm 
being careful. One way to avoid deadlocks is to have a partial order on 
capabilities that is a total order on the subsets of capabilities that are 
acquired together. It's not hard to see that this is sufficient, and I even 
think it's necessary. (In the sense that whenever I have a deadlock-free 
program, such an order exists. I have no proof for that, but that's just for 
lack of trying.)

Now if I acquire capabilities of different types, then the order also has to 
incorporate these different types. So my answer would be no, I think we have to 
allow this.

In fact (but that's for another change) I've even thought about warning when a 
cap. is acquired and it's not ordered with another already-held capability 
(under `-beta` of course).


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72635#1911495 , @aaronpuchert 
wrote:

> @aaron.ballman, I've just noted that one of the `-Wthread-safety-attributes` 
> warnings says 
> 
>
> > //A// attribute can only be applied in a context annotated with 
> > ‘capability(“mutex”)’ attribute
>
> This should be changed then, right? We never enforced the name anyway.


Yeah, something needs to change here. For this diagnostic specifically, we care 
that the type is a capability-annotated type and if it's not, we want to 
diagnose. However, do we want to diagnose when the capability strings are 
different? e.g.,

  typedef int __attribute__((capability("role"))) ThreadRole;
  typedef int __attribute__((capability("noodle"))) Noodle;
  
  Noodle N;
  ThreadRole Data __attribute__((acquired_before(N)));


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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

@aaron.ballman, I've just noted that one of the `-Wthread-safety-attributes` 
warnings says 


> //A// attribute can only be applied in a context annotated with 
> ‘capability(“mutex”)’ attribute

This should be changed then, right? We never enforced the name anyway.


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D72635#1831782 , @eti-p-doray wrote:

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


Happy to do so! I've commit on your behalf in 
5260bc2497bb593ed4a01de5cfe84ed6f7b529b1 
, thank 
you for the patch!


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

[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, ))
 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, ))
 return;
 
-  // Currently, there are 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM with a minor testing request, thank you!




Comment at: clang/test/Sema/attr-capabilities.c:12
-// Test an invalid capability name
-struct __attribute__((capability("wrong"))) IncorrectName {}; // 
expected-warning {{invalid capability name 'wrong'; capability name must be 
'mutex' or 'role'}}
-

I think it would be beneficial to have a test with an arbitrary capability name 
other than role or mutex just to show that it's now explicitly allowed.


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 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 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, ))
 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, ))
 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: