Skip to content

Commit c01f551

Browse files
committed
[transfer-non-sendable] Teach SILIsolationInfo how to handle "look through instructions" when finding actor instances.
From the perspective of the IR, we are changing SILIsolationInfo such that inferring an actor instance means looking at equivalence classes of values where we consider operands to look through instructions to be equivalent to their dest value. The result is that cases where the IR maybe puts in a copy_value or the like, we consider the copy_value to have the same isolation info as using the actor directly. This prevents a class of crashes due to merge failings. Example: ```swift actor MyActor { init() async { init(ns: NonSendableKlass) async { self.k = NonSendableKlass() self.helper(ns) } func helper(_ newK: NonSendableKlass) {} } ``` Incidently, we already had a failing test case from this behavior rather than the one that was the original genesis. Specifically: 1. If a function's SILIsolationInfo is the same as the isolation info of a SILValue, we assume that no transfer actually occurs. 2. Since we were taking too static of a view of actor instances when comparing, we would think that a SILIsolationInfo of a #isolation parameter to as an argument would be different than the ambient's function isolation which is also that same one. So we would emit a transfer non transferrable error if we pass in any parameters of the ambient function into another isolated function. Example: ```swift actor Test { @TaskLocal static var local: Int? func withTaskLocal(isolation: isolated (any Actor)? = #isolation, _ body: (consuming NonSendableValue, isolated (any Actor)?) -> Void) async { Self.$local.withValue(12) { // We used to get these errors here since we thought that body's isolation // was different than the body's isolation. // // warning: sending 'body' risks causing data races // note: actor-isolated 'body' is captured by a actor-isolated closure... body(NonSendableValue(), isolation) } } } ``` rdar://129400019
1 parent baec06e commit c01f551

File tree

6 files changed

+79
-4
lines changed

6 files changed

+79
-4
lines changed

include/swift/SILOptimizer/Utils/SILIsolationInfo.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,16 @@ class ActorInstance {
5555
ActorInstance(SILValue value, Kind kind)
5656
: value(value, std::underlying_type<Kind>::type(kind)) {}
5757

58+
/// We want to look through certain instructions like end_init_ref that have
59+
/// the appropriate actor type but could disguise the actual underlying value
60+
/// that we want to represent our actor.
61+
static SILValue lookThroughInsts(SILValue value);
62+
5863
public:
5964
ActorInstance() : ActorInstance(SILValue(), Kind::Value) {}
6065

6166
static ActorInstance getForValue(SILValue value) {
67+
value = lookThroughInsts(value);
6268
return ActorInstance(value, Kind::Value);
6369
}
6470

@@ -411,6 +417,10 @@ class SILDynamicMergedIsolationInfo {
411417
void printForDiagnostics(llvm::raw_ostream &os) const {
412418
innerInfo.printForDiagnostics(os);
413419
}
420+
421+
SWIFT_DEBUG_DUMPER(dumpForDiagnostics()) {
422+
innerInfo.dumpForDiagnostics();
423+
}
414424
};
415425

416426
} // namespace swift

lib/SILOptimizer/Utils/SILIsolationInfo.cpp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,7 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
394394
// TODO: We really should be doing this based off of an Operand. Then
395395
// we would get the SILValue() for the first element. Today this can
396396
// only mess up isolation history.
397+
397398
return SILIsolationInfo::getActorInstanceIsolated(
398399
SILValue(), isolatedOp->get(), nomDecl);
399400
}
@@ -1100,6 +1101,30 @@ bool SILIsolationInfo::isNonSendableType(SILType type, SILFunction *fn) {
11001101
return !type.isSendable(fn);
11011102
}
11021103

1104+
//===----------------------------------------------------------------------===//
1105+
// MARK: ActorInstance
1106+
//===----------------------------------------------------------------------===//
1107+
1108+
SILValue ActorInstance::lookThroughInsts(SILValue value) {
1109+
if (!value)
1110+
return value;
1111+
1112+
while (auto *svi = dyn_cast<SingleValueInstruction>(value)) {
1113+
if (isa<EndInitLetRefInst>(svi) || isa<CopyValueInst>(svi) ||
1114+
isa<MoveValueInst>(svi) || isa<ExplicitCopyValueInst>(svi) ||
1115+
isa<BeginBorrowInst>(svi) ||
1116+
isa<CopyableToMoveOnlyWrapperValueInst>(svi) ||
1117+
isa<MoveOnlyWrapperToCopyableValueInst>(svi)) {
1118+
value = lookThroughInsts(svi->getOperand(0));
1119+
continue;
1120+
}
1121+
1122+
break;
1123+
}
1124+
1125+
return value;
1126+
}
1127+
11031128
//===----------------------------------------------------------------------===//
11041129
// MARK: SILDynamicMergedIsolationInfo
11051130
//===----------------------------------------------------------------------===//

test/Concurrency/async_task_locals_basic_warnings_bug_isolation.swift

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,10 @@
22
// RUN: %target-swift-frontend -plugin-path %swift-plugin-dir -emit-module -emit-module-path %t/OtherActors.swiftmodule -module-name OtherActors %S/Inputs/OtherActors.swift -disable-availability-checking
33

44
// RUN: %target-swift-frontend -I %t -plugin-path %swift-plugin-dir -disable-availability-checking -strict-concurrency=complete -parse-as-library %s -emit-sil -o /dev/null -verify
5-
// RUN: %target-swift-frontend -I %t -plugin-path %swift-plugin-dir -disable-availability-checking -strict-concurrency=complete -parse-as-library %s -emit-sil -o /dev/null -verify -enable-upcoming-feature RegionBasedIsolation
65

76
// REQUIRES: concurrency
87
// REQUIRES: asserts
98

10-
// FIXME: rdar://125078448 is resolved
11-
// XFAIL: *
12-
139
actor Test {
1410

1511
@TaskLocal static var local: Int?

test/Concurrency/silisolationinfo_inference.sil

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ sil @constructNonSendableKlassIndirect : $@convention(thin) () -> @out NonSendab
3232
sil @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
3333
sil @useUnmanagedNonSendableKlass : $@convention(thin) (@guaranteed @sil_unmanaged NonSendableKlass) -> ()
3434

35+
sil @useActor : $@convention(thin) (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
3536

3637
///////////////////////
3738
// MARK: Basic Tests //
@@ -699,3 +700,24 @@ bb7:
699700
%9999 = tuple ()
700701
return %9999 : $()
701702
}
703+
704+
// CHECK-LABEL: begin running test 1 of 1 on lookthrough_test: sil-isolation-info-inference with: @trace[0]
705+
// CHECK-NEXT: Input Value: %7 = apply %6(%5) : $@convention(thin) (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
706+
// CHECK-NEXT: Isolation: 'argument'-isolated
707+
// CHECK-NEXT: end running test 1 of 1 on lookthrough_test: sil-isolation-info-inference with: @trace[0]
708+
sil [ossa] @lookthrough_test : $@convention(thin) (@owned MyActor) -> @owned NonSendableKlass {
709+
bb0(%0 : @owned $MyActor):
710+
specify_test "sil-isolation-info-inference @trace[0]"
711+
debug_value %0 : $MyActor, let, name "argument"
712+
%1 = end_init_let_ref %0 : $MyActor
713+
%1a = copy_value %1 : $MyActor
714+
%1b = move_value %1a : $MyActor
715+
%1c = begin_borrow %1b : $MyActor
716+
%2 = function_ref @useActor : $@convention(thin) (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
717+
%3 = apply %2(%1c) : $@convention(thin) (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
718+
debug_value [trace] %3 : $NonSendableKlass
719+
end_borrow %1c : $MyActor
720+
destroy_value %1 : $MyActor
721+
destroy_value %1b : $MyActor
722+
return %3 : $NonSendableKlass
723+
}

test/Concurrency/transfernonsendable_initializers.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,13 @@ actor ActorWithSynchronousNonIsolatedInit {
5050
}
5151
}
5252

53+
init(ns: NonSendableKlass) async {
54+
self.k = NonSendableKlass()
55+
self.isolatedHelper(ns)
56+
}
57+
5358
nonisolated func helper(_ newK: NonSendableKlass) {}
59+
func isolatedHelper(_ newK: NonSendableKlass) {}
5460
}
5561

5662
func initActorWithSyncNonIsolatedInit() {

test/Concurrency/transfernonsendable_ownership.swift

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,3 +105,19 @@ func testInOutError(_ x: inout Klass) async {
105105
// expected-note @-1 {{sending global actor 'CustomActor'-isolated 'x' to main actor-isolated global function 'transferToMain'}}
106106
_ = consume x // expected-note {{consumed here}}
107107
} // expected-note {{used here}}
108+
109+
actor ActorTestCase {
110+
let k = Klass()
111+
112+
// TODO: This crashes the compiler since we attempt to hop_to_executor to the
113+
// value that is materialized onto disk.
114+
//
115+
// consuming func test() async {
116+
// await transferToMain(k)
117+
// }
118+
119+
borrowing func test2() async {
120+
await transferToMain(k) // expected-warning {{sending 'self.k' risks causing data races}}
121+
// expected-note @-1 {{sending 'self'-isolated 'self.k' to main actor-isolated global function 'transferToMain' risks causing data races between main actor-isolated and 'self'-isolated uses}}
122+
}
123+
}

0 commit comments

Comments
 (0)