[llvm-branch-commits] Thread Safety Analysis: Add guarded_by_any and pt_guarded_by_any (PR #185173)

[Marco Elver via llvm-branch-commits]

melver wrote:

> > I thought of retrofitting `guarded_by` with this new semantics initially - only reason I didn't immediately do it, is because it seems a little unintuitive to the casual user, who might believe it's the same as stacking the attributes multiple times (as the other attributes' behaviour).
> 
> Yeah, the motivation is clear to me. But either way we have one attribute that behaves different, and "any" could also be seen as misleading, because it's "all exclusive / any shared". I've been thinking about what makes this attribute different that might justify different behavior, and one thing stood out: while most other attributes are either related to exclusive or shared capabilities, this attributes has an overloaded meaning for both: it allows read accesses for shared capabilities and write access for exclusive capabilities.
> 
> However, an attribute like `requires_shared_capability` could also have "any" semantics. If you need "all", you could simply use separate attributes. This makes checking a bit more difficult, because we don't know about any of the locks given that they're actually held. However, if you have these "any shared" semantics on `guarded_by` (or a new attribute), you will probably sooner or later also have functions that can be called with any of the given locks.

Yeah, the requirements on code are usually derived from the requirements on data. However, code appears to not require this ("any") flexibility all that much in my experience, whereas data does.

Though it is not entirely clear to me if `requires_shared_capability(a, b)` is actually sound if we start applying the same rules ("hold any of a,b"), because as you say unlike `guarded_by`, these function-level attributes explicitly state ownership. Changing this would also break backwards compatibility.

> I've also been thinking about whether we should actually give multiple independent `guarded_by` attributes the same "all exclusive / any shared" behavior. This would probably make implementation easier and perhaps ease your concerns about consistency. It would certainly raise concerns about breaking the meaning of existing code, but I wonder how often multiple `guarded_by` are used and what the intended meaning is. (In case any of the capabilities actually allow shared locks, which not all capabilities do.)

As for the last point, that'd be a backwards-compatibility break - I don't think we should do so. And I would want to retain at least one mechanism to say "all these locks must be held" - stacking attributes would be very explicit and let us continue to do so.

As for how to proceed: I'm going to fold the "any" semantics into the `guarded_by` attribute and drop `guarded_by_any` (this PR). From all this it's sufficiently ambiguous if introducing a new attribute is actually better or not, that I'm going to err on the side of simplicity (no new attribute, no backwards compatibility breaks). I'll try to push the updated PR next week.

https://github.com/llvm/llvm-project/pull/185173