Allow arbitrary capability name in Thread Safety Analysis

[eti-p-doray]

Summary

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.
As an example copied from:
https://chromium-review.googlesource.com/c/chromium/src/+/1948098/19/base/sequence_checker_unittest.nc#33

int counter_ GUARDED_BY_CONTEXT(sequence_checker_);
// Member access without sequence_checker_ assertion.
++counter_;
Should yield: "fatal error: writing variable 'counter_' requires holding context 'sequence_checker_' exclusively"

[aaron.ballman]

Can you give some examples of code where you think the diagnostics are inappropriate? One of the goals of role-based capabilities is for you to name the capabilities as you see fit, so it's possible that there is a different way for us to surface those diagnostics for your situation that doesn't involve adding another one-off name for capabilities.

[eti-p-doray]

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?

[aaron.ballman]

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

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?

I had to hop in a time machine to figure out what we were thinking with restricting the names to just role and mutex and did not see any reasonable rationale for the restriction and I'm in favor of lifting it now. I think the correct way forward here is to remove the diagnostic (allowing arbitrary capability names), remove the isRole() and isMutex() accessors from the attribute definition in Attr.td (they're unused), and then update the documentation to make it more clear that we allow arbitrary names for capabilities.

Adding a few more people who use these attributes to see if there are concerns with this approach.

[aaronpuchert]

Hmm, I have been wondering about this as well. The way I see it, all of these things are what we call capabilities, and we treat them all the same. The names are just meant to make warning messages more readable, because what the analysis considers a capability, the user might know as a mutex, or role, or sequence.

I think I'll read a bit of code to see if this is really true and there are no functional differences between the differently named capabilities, but other than that I've no objections to allowing arbitrary names. I'm trying to come up with potential problems caused by opening the flood gates, but frankly I don't see any.

Edit: I've gone through ThreadSafety.cpp and AnalysisBasedWarnings.cpp and it seems that we indeed only write the name into the diagnostic.

[aaron.ballman]

In D72635#1825219, @aaronpuchert wrote:

Hmm, I have been wondering about this as well. The way I see it, all of these things are what we call capabilities, and we treat them all the same. The names are just meant to make warning messages more readable, because what the analysis considers a capability, the user might know as a mutex, or role, or sequence.

Yup, that was intentional. We started out with thread safety analysis using "lock" terminology everywhere, but then switched to "capability" analysis which is roughly the same thing using more generic terms.

I think I'll read a bit of code to see if this is really true and there are no functional differences between the differently named capabilities, but other than that I've no objections to allowing arbitrary names. I'm trying to come up with potential problems caused by opening the flood gates, but frankly I don't see any.

Edit: I've gone through ThreadSafety.cpp and AnalysisBasedWarnings.cpp and it seems that we indeed only write the name into the diagnostic.

Thank you for double-checking -- I had come to the same conclusion myself.

[eti-p-doray]

Allow arbitrary capability

[eti-p-doray]

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

[aaron.ballman]

LGTM with a minor testing request, thank you!

clang/test/Sema/attr-capabilities.c

12

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.

[eti-p-doray]

Added test with custom capability name.

[eti-p-doray]

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

[aaron.ballman]

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!

[aaronpuchert]

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

[aaron.ballman]

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

[aaronpuchert]

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

[aaron.ballman]

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!

[aaronpuchert]

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.

clang/include/clang/Basic/Attr.td

 
 def Capability : InheritableAttr {
   let Spellings = [Clang<"capability", 0>, Clang<"shared_capability", 0>];
   let Subjects = SubjectList<[Record, TypedefName], ErrorDiag>;
   let Args = [StringArgument<"Name">];
   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 {
   let Spellings = [Clang<"assert_capability", 0>,
                    Clang<"assert_shared_capability", 0>];
   let Subjects = SubjectList<[Function]>;
   let LateParsed = 1;
   let TemplateDependent = 1;

clang/include/clang/Basic/DiagnosticSemaKinds.td

def note_unguarded_available_silence : Note<
   "enclose %0 in %select{an @available|a __builtin_available}1 check to silence"
   " this warning">;
 def warn_at_available_unchecked_use : Warning<
   "%select{@available|__builtin_available}0 does not guard availability here; "
   "use if (%select{@available|__builtin_available}0) instead">,
   InGroup<DiagGroup<"unsupported-availability-guard">>;
 
 // Thread Safety Attributes
-def warn_invalid_capability_name : Warning<
-  "invalid capability name '%0'; capability name must be 'mutex' or 'role'">,
-  InGroup<ThreadSafetyAttributes>, DefaultIgnore;
 def warn_thread_attribute_ignored : Warning<
   "ignoring %0 attribute because its argument is invalid">,
   InGroup<ThreadSafetyAttributes>, DefaultIgnore;
 def warn_thread_attribute_not_on_non_static_member : Warning<
   "%0 attribute without capability arguments can only be applied to non-static "
   "methods of a class">,
   InGroup<ThreadSafetyAttributes>, DefaultIgnore;
 def warn_thread_attribute_not_on_capability_member : Warning<

clang/lib/Sema/SemaDeclAttr.cpp

static void handleCapabilityAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   // For backward compatibility, any capability which has no specified string
   // literal will be considered a "mutex."
   StringRef N("mutex");
   SourceLocation LiteralLoc;
   if (AL.getKind() == ParsedAttr::AT_Capability &&
       !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));
 }
 
 static void handleAssertCapabilityAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   SmallVector<Expr*, 1> Args;
   if (!checkLockFunAttrCommon(S, D, AL, Args))
     return;
 

clang/test/Sema/attr-capabilities.c

 // 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 {};
 
 // Put capability attributes on unions
 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}}
 int Test3 __attribute__((acquire_capability("test3")));  // expected-warning {{'acquire_capability' attribute only applies to functions}}
 int Test4 __attribute__((try_acquire_capability("test4"))); // expected-error {{'try_acquire_capability' attribute only applies to functions}}
 int Test5 __attribute__((release_capability("test5"))); // expected-warning {{'release_capability' attribute only applies to functions}}
 
 struct __attribute__((capability(12))) Test3 {}; // expected-error {{'capability' attribute requires a string}}