Skip to content

Commit c846c22

Browse files
committed
[rbi] Refactor getUnderlyingTrackedValue so that for addresses we return 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.
1 parent 8f458cd commit c846c22

File tree

5 files changed

+563
-225
lines changed

5 files changed

+563
-225
lines changed

include/swift/SILOptimizer/Analysis/RegionAnalysis.h

Lines changed: 72 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ class BlockPartitionState {
108108

109109
class TrackableValue;
110110
class TrackableValueState;
111+
struct TrackableValueLookupResult;
111112

112113
enum class TrackableValueFlag {
113114
/// Base value that says a value is uniquely represented and is
@@ -272,6 +273,25 @@ class regionanalysisimpl::TrackableValue {
272273
}
273274
};
274275

276+
/// A class that contains both a lookup value as well as extra metadata about
277+
/// properties of the original value that we looked up up from that we
278+
/// discovered as we searched for the lookup value.
279+
struct regionanalysisimpl::TrackableValueLookupResult {
280+
/// The actual value that we are tracking.
281+
///
282+
/// If we are tracking a Sendable address that has a non-Sendable base, this
283+
/// will be an empty TrackableValue.
284+
TrackableValue value;
285+
286+
/// If we are tracking an address, this is the base trackable value that is
287+
/// being tracked. If the base is a Sendable value, then this will be an empty
288+
/// TrackableValue.
289+
std::optional<TrackableValue> base;
290+
291+
void print(llvm::raw_ostream &os) const;
292+
SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
293+
};
294+
275295
class RegionAnalysis;
276296

277297
class RegionAnalysisValueMap {
@@ -282,6 +302,8 @@ class RegionAnalysisValueMap {
282302
using Region = PartitionPrimitives::Region;
283303
using TrackableValue = regionanalysisimpl::TrackableValue;
284304
using TrackableValueState = regionanalysisimpl::TrackableValueState;
305+
using TrackableValueLookupResult =
306+
regionanalysisimpl::TrackableValueLookupResult;
285307

286308
private:
287309
/// A map from the representative of an equivalence class of values to their
@@ -301,18 +323,47 @@ class RegionAnalysisValueMap {
301323

302324
/// State that the value -> representative computation yields to us.
303325
struct UnderlyingTrackedValueInfo {
326+
/// The equivalence class value that we found that should be merged into
327+
/// regions.
328+
///
329+
/// Always set to a real value.
304330
SILValue value;
305331

306-
explicit UnderlyingTrackedValueInfo(SILValue value) : value(value) {}
332+
/// The actual base value that we found if we were looking for an address
333+
/// equivilance class and had a non-Sendable base. If we have an object or
334+
/// we do not have a separate base, this is SILValue().
335+
SILValue base;
307336

308-
UnderlyingTrackedValueInfo() : value() {}
337+
/// Constructor for use if we only have either an object or an address
338+
/// equivalence class that involves a complete non-Sendable path.
339+
explicit UnderlyingTrackedValueInfo(SILValue value) : value(value), base() {
340+
assert(value);
341+
}
342+
343+
/// Constructor for use with addresses only where we have either:
344+
///
345+
/// 1. A sendable address that is used but that has a non-Sendable base that
346+
/// we have to insert requires for.
347+
///
348+
/// 2. A non-Sendable address that is used but that has a separate
349+
/// non-Sendable base due to an access path chain that has a split in
350+
/// between the two due to the non-Sendable address being projected out of
351+
/// an intervening sendable struct. The struct can be Sendable due to things
352+
/// like being global actor isolated or by being marked @unchecked Sendable.
353+
explicit UnderlyingTrackedValueInfo(SILValue value, SILValue base)
354+
: value(value), base(base) {
355+
assert(value);
356+
assert(base);
357+
}
358+
UnderlyingTrackedValueInfo() : value(), base() {}
309359

310360
UnderlyingTrackedValueInfo(const UnderlyingTrackedValueInfo &newVal)
311-
: value(newVal.value) {}
361+
: value(newVal.value), base(newVal.base) {}
312362

313363
UnderlyingTrackedValueInfo &
314364
operator=(const UnderlyingTrackedValueInfo &newVal) {
315365
value = newVal.value;
366+
base = newVal.base;
316367
return *this;
317368
}
318369

@@ -355,10 +406,16 @@ class RegionAnalysisValueMap {
355406
void print(llvm::raw_ostream &os) const;
356407
SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
357408

358-
TrackableValue
409+
TrackableValueLookupResult
359410
getTrackableValue(SILValue value,
360411
bool isAddressCapturedByPartialApply = false) const;
361412

413+
private:
414+
TrackableValue
415+
getTrackableValueHelper(SILValue value,
416+
bool isAddressCapturedByPartialApply = false) const;
417+
418+
public:
362419
/// An actor introducing inst is an instruction that doesn't have any
363420
/// non-Sendable parameters and produces a new value that has to be actor
364421
/// isolated.
@@ -370,7 +427,8 @@ class RegionAnalysisValueMap {
370427

371428
private:
372429
std::optional<TrackableValue> getValueForId(Element id) const;
373-
std::optional<TrackableValue> tryToTrackValue(SILValue value) const;
430+
std::optional<TrackableValueLookupResult>
431+
tryToTrackValue(SILValue value) const;
374432
TrackableValue
375433
getActorIntroducingRepresentative(SILInstruction *introducingInst,
376434
SILIsolationInfo isolation) const;
@@ -392,6 +450,15 @@ class RegionAnalysisValueMap {
392450
UnderlyingTrackedValueInfo
393451
getUnderlyingTrackedValueHelper(SILValue value) const;
394452

453+
/// A helper function that performs the actual getUnderlyingTrackedValue
454+
/// computation that is cached in getUnderlyingTrackedValue(). Please never
455+
/// call this directly! Only call it from getUnderlyingTrackedValue.
456+
UnderlyingTrackedValueInfo
457+
getUnderlyingTrackedValueHelperObject(SILValue value) const;
458+
459+
UnderlyingTrackedValueInfo
460+
getUnderlyingTrackedValueHelperAddress(SILValue value) const;
461+
395462
UnderlyingTrackedValueInfo getUnderlyingTrackedValue(SILValue value) const {
396463
// Use try_emplace so we only construct underlying tracked value info on
397464
// success and only lookup once in the hash table.

include/swift/SILOptimizer/Utils/SILIsolationInfo.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -442,10 +442,18 @@ class SILIsolationInfo {
442442
/// TODO: Fix the type checker.
443443
static bool isNonSendableType(SILType type, SILFunction *fn);
444444

445+
static bool isSendableType(SILType type, SILFunction *fn) {
446+
return !isNonSendableType(type, fn);
447+
}
448+
445449
static bool isNonSendableType(SILValue value) {
446450
return isNonSendableType(value->getType(), value->getFunction());
447451
}
448452

453+
static bool isSendableType(SILValue value) {
454+
return !isNonSendableType(value);
455+
}
456+
449457
bool hasSameIsolation(ActorIsolation actorIsolation) const;
450458

451459
/// Returns true if \p this and \p other have the same isolation. It allows

0 commit comments

Comments
 (0)