Page MenuHomePhabricator

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

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.Tue, Jan 21, 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!