[rbi] Teach RBI how to handle non-Sendable bases of Sendable values #80745
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Specifically, 1. UseDefChainVisitor::actorIsolation is dead. I removed it to prevent any confusion/thoughts that it actually found isolation. I also removed it from UnderlyingTrackedValue since that was the only place where we were using it. I left UnderlyingTrackedValue there in case I need to add additional state there in the future. 2. Now that UseDefChainVisitor is only used to find the base of a value (while not looking through Sendable addresses), I renamed it to AddressBaseComputingVisitor. 3. I renamed UseDefChainVisitor::isMerge to isProjectedFromAggregate. That is actually what we use it for. I also added a comment explaining what it is used for.
Due to compile time issues, I added a cache into getUnderlyingTrackedValue(). This caused an iterator invalidation issue when we recursed in the case when we had an underlying object since we would recurse into getUnderlyingTrackedValue() instead of getUnderlyingTrackedValueHelper() potentially causing us to cache another value and thus causing the underlying DenseMap to expand. Now we instead just call getUnderlyingTrackedValueHelper() so that we avoid the invalidation issue. This may cause us to use slightly more compile time but we are still only ever going to compute the underlying value once for any specific value.
…ableValue(). I also added some basic tests of its functionality. I am doing this in preparation for making some more invasive changes to getTrackableValue and I want to be able to test it out very specifically in SIL.
…urn both a value and a base in certain situations.
Previously, when we saw any Sendable type and attempted to look up an underlying
tracked value, we just bailed. This caused an issue in situations like the
following where we need to emit an error:
```swift
func test() {
var x = 5
Task.detached { x += 1 }
print(x)
}
```
The problem with the above example is that despite value in x being Sendable,
'x' is actually in a non-Sendable box. We are passing that non-Sendable box into
the detached task by reference causing a race against the read from the
non-Sendable box later in the function. In SE-0414, this is explicitly banned in
the section called "Accessing Sendable fields of non-Sendable types after weak
transferring". In this example, the box is the non-Sendable type and the value
stored in the box is the Sendable field.
To properly represent this, we need to change how the underlying object part of
our layering returns underlying objects and vends TrackableValues to the actual
analysis for addresses. NOTE: We leave the current behavior alone for SIL
objects.
By doing this, in situations like the above, despite have a Sendable value (the
integer), we are able to ensure that we require that the non-Sendable box
containing the integer is not used after we have sent it into the other Task
despite us not actually using the box directly.
Below I describe the representation change in more detail and describe the
various cases here. In this commit, I only change the representation and do not
actually use the new base information. I do that in the next commit to make this
change easier for others to read and review. I made sure that change was NFC by
leaving RegionAnalysis.cpp:727 returning an optional.none if the value found was
a Sendable value.
----
The way we modify the representation is that we instead of just returning a
single TrackedValue return a pair of tracked values, one for the base and one
for the "value". We return this pair in what is labeled a
"TrackableValueLookupResult":
```c++
struct TrackableValueLookupResult {
TrackableValue value;
std::optional<TrackableValue> base;
TrackableValueLookupResult(TrackableValue value)
: value(value), base() {}
TrackableValueLookupResult(TrackableValue value, TrackableValue base)
: value(value), base(base) {}
};
```
In the case where we are accessing a projection path out of a non-Sendable type
that contains all non-Sendable fields, we do not do anything different than we
did previously. We just walk up from use->def until we find the access path base
which we use as the representative of the leaf of the chain and return
TrackableValueLookupResult(access path base).
In the case where we are accessing a Sendable leaf type projected from a
non-Sendable base, we store the leaf type as our value and return the actual
non-Sendable base in TrackableValueLookupResult. Importantly this ensures that
even though our Sendable value will be ignored by the rest of the analysis, the
rest of the analysis will ensure that our base is required if our base is a var
that had been escaped into a closure by reference.
In the case where we are accessing a non-Sendable leaf type projected from a
Sendable type (which we may have continued to be projected subsequently out of
additional Sendable types or a non-Sendable type), we make the last type on the
projection path before the Sendable type, the value of the leaf type. We return
the eventual access path base as our underlying value base. The logic here is
that since we are dealing with access paths, our access path can only consist of
projections into a recursive value type (e.x.: struct/tuple/enum... never a
class). The minute that we hit a pointer or a class, we will no longer be along
the access path since we will be traversing a non-contiguous piece of
memory (consider a class vs the class's storage) and the traversal from use->def
will stop. Thus, we know that there are only two ways we can get a field in that
value type to be Sendable and have a non-Sendable field:
1. The struct can be @unchecked Sendable. In such a case, we want to treat the
leaf field as part of its own disconnected region.
2. The struct can be global actor isolated. In such a case, we want to treat the
leaf field as part of the global actor's region rather than whatever actor.
The reason why we return the eventual access path base as our tracked value base
is that we want to ensure that if the var value had been escaped by reference,
we can require that the var not be sent since we are going to attempt to access
state from the var in order to get the global actor guarded struct that we are
going to attempt to extract our non-Sendable leaf value out of.
There are a few major changes here: 1. We now return a TrackableValue from getTrackableValue() if we have either a non-Sendable value or a non-Sendable base. This means that we /will/ return TrackableValues that may have a Sendable value or a Sendable base. To make it easier to work with this, I moved the isSendable check and the do I have a base check into PartitionOpBuilder. So, most of the actual code around emitting values does not need to reason about this. They can just call addRequire or addSend and pass in either TrackableValue::value or TrackableValue::base without needing to check if the former is non-Sendable or if the latter is non-Sendable and non-nil. 2. I searched all of the places where we were grabbing trackable values and inserted require checks for the base value as appropriate. Both of these together have prevented the code from becoming too heavy. This fixes https://forums.swift.org/t/lets-debug-missing-rbi-data-race-diagnostics/78910 rdar://149019222
I am doing this so I can mark requires as being on a mutable non-Sendable base from a Sendable value. I also took this as an opportunity to compress the size of PartitionOp to be 24 bytes instead of 40 bytes.
…alues that are projected from non-Sendable bases. Specifically, we only do this if the base is a let or if it is a var but not captured by reference. rdar://149019222
|
@swift-ci smoke test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains a fix for #80014. At a high level this required a bit of changing to how we model underlying values in RBI. Specifically, we needed to begin providing the higher layers of the system access both to the underlying equivalence class value that a value is from and also potentially a base value that the equivalence class is loaded from (if it is an address). This allows us to model Sendable values that have non-Sendable bases and ensure that we require the base so that we properly error on cases like this:
even though technically the actual use is of a Sendable value.
This change also exposed some deficiencies in the way that we modeled and handled loading Sendable fields from non-Sendable types after the non-Sendable type was sent. I fixed these issues as well.
Finally, I did a little refactoring so that any checking around Sendability and the like are hidden in the builder that takes in these TrackableValues rather than in the translation layer that uses the builder and feeds values into the builder. I did this to eliminate a class of bugs where a user becomes confused on how to handle TrackableValues that could have a Sendable value within it.
rdar://149019222