This is an archive of the discontinued LLVM Phabricator instance.

Allow arbitrary capability name in Thread Safety Analysis
ClosedPublic

Authored by eti-p-doray on Jan 13 2020, 11:31 AM.

Details

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"

Diff Detail

Event Timeline

eti-p-doray created this revision.Jan 13 2020, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 11:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 edited the summary of this revision. (Show Details)Jan 15 2020, 1:01 PM

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 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 added a comment.EditedJan 16 2020, 2:40 PM

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.

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 retitled this revision from Add "context" capability to Thread Safety Analysis to Allow arbitrary capability name in Thread Safety Analysis.

Allow arbitrary capability

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

aaron.ballman accepted this revision.Jan 18 2020, 6:32 AM

LGTM with a minor testing request, thank you!

clang/test/Sema/attr-capabilities.c
12 ↗(On Diff #238782)

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.

This revision is now accepted and ready to land.Jan 18 2020, 6:32 AM

Added test with custom capability name.

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

eti-p-doray marked an inline comment as done.Jan 21 2020, 10:15 AM

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!

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

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

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!

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.