[6.2][rbi] Teach RBI how to handle non-Sendable bases of Sendable values #80840
Conversation
This file contains 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
(cherry picked from commit 69ee33a)
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. (cherry picked from commit 98984a2)
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. (cherry picked from commit 5499734)
…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. (cherry picked from commit 8f458cd)
…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.
(cherry picked from commit c846c22)
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 (cherry picked from commit 6d8b9b0)
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. (cherry picked from commit a045c98)
…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 (cherry picked from commit 23b6937)
gottesmm
changed the title
|
@swift-ci test |
hborla
approved these changes
Apr 16, 2025
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.
Explanation: This involves fixing a serious bug that was reported on the swift forums: https://forums.swift.org/t/lets-debug-missing-rbi-data-race-diagnostics/78910/6. At a high level the problem is that previously when we saw a use of a Sendable value, we just ignored the use. This becomes an issue if we are extracting the Sendable value from a non-Sendable value using static fields. Often times this would reduce the access to the Sendable value to loads and projections which are look through instructions and do not act as actual uses of the underlying non-Sendable value. If we had already sent the non-Sendable value to another isolation domain by reference this would result in a race:
In words, the above example shows a non-Sendable box containing an Integer value. We send that non-Sendable value into a detached Task by reference so we can update its value to 1. In the mean time, we attempt to update x to 2 in the original Task resulting in a race.
NOTE: If we extracted the Sendable value from the non-Sendable value using a computed property, we would in the above case recognize the computed property use as a use of the non-Sendable base and emit an error. This only occurs if we project out using static properties.
To fix this, I had to make some changes to the RBI "machine", namely making it so that when we compute the equivalence class that a specific value belongs to, we also return potentially a base value. From the perspective of RBI, I hid that we are doing this by making it so that we no longer manipulate underlying values directly. Instead, we always work with them in pairs and in PartitionOpBuilder itself, we handle the pair's elements as appropriate. The truth is we only use the base in one way: when we require, we also require the base if it is non-Sendable. The rest of the methods on PartitionOpBuilder just ignores the base.
There are two potential cases where we get a base:
Scope: Affects how we find underlying equivalence class values in RBI and makes it so we also track the base. Changes RBI so that the change does not impact the flow of how we translate instructions by having the builder struct that creates partition ops be the place that we reason about bases. Also adds the ability to test the map from value -> equivalence class directly.
Issue: rdar://149019222
Original PR: 23b6937
Risk: Medium/Low. This change does do a big of surgery to RBI inherently increasing the risk of unintentional bugs. But we think that fixing this concurrency hole is important enough and the additional testing provided by the direct testing of the changed functionality mitigates that issue.
Testing: Added tests to the test suite that test out the specific test cases that broke. Validated that we did not have to change any already passing tests (that is with all current tests this doesn't change compiler output). I also added new tests where the compiler directly dumps out the value/base that it discovers. This allows for this behavior change to be tested directly without relying on other parts of the compiler working correctly.
Reviewer: @hborla