Skip to content

Commit 08eb6ec

Browse files
DougGregorDougGregor
authored
Merge pull request swiftlang#39542 from ktoso/remote-calls-bad
[Distributed] Fix when we invoke thunk, base it on typesystem implicit effects
2 parents dd2ff87 + bf0681d commit 08eb6ec

File tree

7 files changed

+96
-35
lines changed

7 files changed

+96
-35
lines changed

‎include/swift/AST/Expr.h

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -307,12 +307,13 @@ class alignas(8) Expr : public ASTAllocated<Expr> {
307307
NumCaptures : 32
308308
);
309309

310-
SWIFT_INLINE_BITFIELD(ApplyExpr, Expr, 1+1+1+1+1,
310+
SWIFT_INLINE_BITFIELD(ApplyExpr, Expr, 1+1+1+1+1+1,
311311
ThrowsIsSet : 1,
312312
Throws : 1,
313313
ImplicitlyAsync : 1,
314314
ImplicitlyThrows : 1,
315-
NoAsync : 1
315+
NoAsync : 1,
316+
ShouldApplyDistributedThunk : 1
316317
);
317318

318319
SWIFT_INLINE_BITFIELD_EMPTY(CallExpr, ApplyExpr);
@@ -1206,7 +1207,9 @@ class DeclRefExpr : public Expr {
12061207
/// which are cross-actor invoked, because such calls actually go over the
12071208
/// transport/network, and may throw from this, rather than the function
12081209
/// implementation itself..
1209-
bool isImplicitlyThrows() const { return Bits.DeclRefExpr.IsImplicitlyThrows; }
1210+
bool isImplicitlyThrows() const {
1211+
return Bits.DeclRefExpr.IsImplicitlyThrows;
1212+
}
12101213

12111214
/// Set whether this reference must account for a `throw` occurring for reasons
12121215
/// other than the function implementation itself throwing, e.g. an
@@ -4391,7 +4394,7 @@ class ApplyExpr : public Expr {
43914394
///
43924395
/// where the new closure is declared to be async.
43934396
///
4394-
/// When the application is implciitly async, the result describes
4397+
/// When the application is implicitly async, the result describes
43954398
/// the actor to which we need to need to hop.
43964399
Optional<ImplicitActorHopTarget> isImplicitlyAsync() const {
43974400
if (!Bits.ApplyExpr.ImplicitlyAsync)
@@ -4419,6 +4422,15 @@ class ApplyExpr : public Expr {
44194422
Bits.ApplyExpr.ImplicitlyThrows = flag;
44204423
}
44214424

4425+
/// Informs IRGen to that this expression should be applied as its distributed
4426+
/// thunk, rather than invoking the function directly.
4427+
bool shouldApplyDistributedThunk() const {
4428+
return Bits.ApplyExpr.ShouldApplyDistributedThunk;
4429+
}
4430+
void setShouldApplyDistributedThunk(bool flag) {
4431+
Bits.ApplyExpr.ShouldApplyDistributedThunk = flag;
4432+
}
4433+
44224434
ValueDecl *getCalledValue() const;
44234435

44244436
static bool classof(const Expr *E) {

‎lib/SILGen/SILGenApply.cpp

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1117,12 +1117,8 @@ class SILGenApply : public Lowering::ExprVisitor<SILGenApply> {
11171117

11181118
// Otherwise, we have a statically-dispatched call.
11191119
auto constant = SILDeclRef(e->getDecl());
1120-
if (e->getDecl()->getAttrs().hasAttribute<DistributedActorAttr>()) {
1121-
// we're calling a distributed function, only in cross-actor situations
1122-
// does this actually need to call the thunk.
1123-
if (!selfApply) {
1124-
constant = constant.asDistributed(true);
1125-
}
1120+
if (callSite && callSite->shouldApplyDistributedThunk()) {
1121+
constant = constant.asDistributed(true);
11261122
} else {
11271123
constant = constant.asForeign(
11281124
!isConstructorWithGeneratedAllocatorThunk(e->getDecl())

‎lib/Sema/TypeCheckConcurrency.cpp

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1003,14 +1003,16 @@ namespace {
10031003
///
10041004
/// and we reach up to mark the CallExpr.
10051005
void markNearestCallAsImplicitly(
1006-
Optional<ImplicitActorHopTarget> setAsync, bool setThrows = false) {
1006+
Optional<ImplicitActorHopTarget> setAsync,
1007+
bool setThrows = false, bool setDistributedThunk = false) {
10071008
assert(applyStack.size() > 0 && "not contained within an Apply?");
10081009

10091010
const auto End = applyStack.rend();
10101011
for (auto I = applyStack.rbegin(); I != End; ++I)
10111012
if (auto call = dyn_cast<CallExpr>(*I)) {
10121013
if (setAsync) call->setImplicitlyAsync(*setAsync);
10131014
if (setThrows) call->setImplicitlyThrows(true);
1015+
if (setDistributedThunk) call->setShouldApplyDistributedThunk(true);
10141016
return;
10151017
}
10161018
llvm_unreachable("expected a CallExpr in applyStack!");
@@ -1667,7 +1669,6 @@ namespace {
16671669
ThrowsMarkingResult tryMarkImplicitlyThrows(SourceLoc declLoc,
16681670
ConcreteDeclRef concDeclRef,
16691671
Expr* context) {
1670-
16711672
ValueDecl *decl = concDeclRef.getDecl();
16721673
ThrowsMarkingResult result = ThrowsMarkingResult::NotFound;
16731674

@@ -1869,8 +1870,11 @@ namespace {
18691870
}
18701871

18711872
switch (contextIsolation) {
1872-
case ActorIsolation::ActorInstance:
1873-
case ActorIsolation::DistributedActorInstance: {
1873+
case ActorIsolation::DistributedActorInstance:
1874+
markNearestCallAsImplicitly(/*setAsync*/None, /*setThrows*/false,
1875+
/*setDistributedThunk*/true);
1876+
LLVM_FALLTHROUGH;
1877+
case ActorIsolation::ActorInstance: {
18741878
auto result = tryMarkImplicitlyAsync(
18751879
loc, valueRef, context,
18761880
ImplicitActorHopTarget::forGlobalActor(globalActor));
@@ -2277,7 +2281,10 @@ namespace {
22772281
} else if (func->isDistributed()) {
22782282
tryMarkImplicitlyAsync(memberLoc, memberRef, context,
22792283
ImplicitActorHopTarget::forInstanceSelf());
2280-
tryMarkImplicitlyThrows(memberLoc, memberRef, context);
2284+
tryMarkImplicitlyThrows(memberLoc, memberRef, context);
2285+
markNearestCallAsImplicitly(/*setAsync*/None, /*setThrows*/false,
2286+
/*setDistributedThunk*/true);
2287+
22812288
} else {
22822289
// neither static or distributed, apply full distributed isolation
22832290
ctx.Diags.diagnose(memberLoc, diag::distributed_actor_isolated_method)
@@ -2362,8 +2369,7 @@ namespace {
23622369
}
23632370

23642371
// It wasn't a distributed func, so ban the access
2365-
// TODO(distributed): handle subscripts here too
2366-
if (auto var = dyn_cast<VarDecl>(member)) {
2372+
if (isPropOrSubscript(member)) {
23672373
ctx.Diags.diagnose(
23682374
memberLoc, diag::distributed_actor_isolated_non_self_reference,
23692375
member->getDescriptiveKind(), member->getName());
@@ -2379,6 +2385,8 @@ namespace {
23792385
if (isolation.getActorType()->isDistributedActor() &&
23802386
!isolatedActor.isPotentiallyIsolated) {
23812387
tryMarkImplicitlyThrows(memberLoc, memberRef, context);
2388+
markNearestCallAsImplicitly(/*setAsync*/None, /*setThrows*/false,
2389+
/*setDistributedThunk*/true);
23822390
}
23832391

23842392
if (implicitAsyncResult == AsyncMarkingResult::FoundAsync)

‎test/Distributed/Runtime/distributed_actor_dynamic_remote_func.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ struct FakeTransport: ActorTransport {
5454
fatalError("not implemented:\(#function)")
5555
}
5656

57-
func resolve<Act>(_ identity: Act.ID, as actorType: Act.Type)
57+
func resolve<Act>(_ identity: AnyActorIdentity, as actorType: Act.Type)
5858
throws -> Act?
5959
where Act: DistributedActor {
6060
return nil
@@ -95,7 +95,7 @@ func test_remote() async throws {
9595
let address = ActorAddress(parse: "")
9696
let transport = FakeTransport()
9797

98-
let worker = try LocalWorker(resolve: .init(address), using: transport)
98+
let worker = try LocalWorkerresolve(.init(address), using: transport)
9999
let x = try await worker.function()
100100
print("call: \(x)")
101101
// CHECK: call: _cluster_remote_function():

‎test/Distributed/Runtime/distributed_actor_local.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ struct FakeTransport: ActorTransport {
4949
fatalError("not implemented \(#function)")
5050
}
5151

52-
func resolve<Act>(_ identity: Act.ID, as actorType: Act.Type) throws -> Act?
52+
func resolve<Act>(_ identity: AnyActorIdentity, as actorType: Act.Type) throws -> Act?
5353
where Act: DistributedActor {
5454
return nil
5555
}
@@ -75,7 +75,7 @@ func test_initializers() {
7575
let transport = FakeTransport()
7676

7777
_ = SomeSpecificDistributedActor(transport: transport)
78-
_ = try! SomeSpecificDistributedActor(resolve: .init(address), using: transport)
78+
_ = try! SomeSpecificDistributedActor.resolve(.init(address), using: transport)
7979
}
8080

8181
@available(SwiftStdlib 5.5, *)

‎test/Distributed/Runtime/distributed_actor_remote_functions.swift

Lines changed: 57 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %target-run-simple-swift(-Xfrontend -enable-experimental-distributed -parse-as-library) | %FileCheck %s --dump-input=always
1+
// RUN: %target-run-simple-swift(-Xfrontend -disable-availability-checking -Xfrontend -enable-experimental-distributed -parse-as-library) | %FileCheck %s --dump-input=always
22

33
// REQUIRES: executable_test
44
// REQUIRES: concurrency
@@ -11,8 +11,6 @@
1111
// rdar://77798215
1212
// UNSUPPORTED: OS=windows-msvc
1313

14-
// REQUIRES: rdar78290608
15-
1614
import _Distributed
1715
import _Concurrency
1816

@@ -43,6 +41,31 @@ distributed actor SomeSpecificDistributedActor {
4341
"local(\(#function))"
4442
}
4543

44+
distributed func callTaskSelf_inner() async throws -> String {
45+
"local(\(#function))"
46+
}
47+
distributed func callTaskSelf() async -> String {
48+
do {
49+
return try await Task {
50+
let called = try await callTaskSelf_inner() // shouldn't use the distributed thunk!
51+
return "local(\(#function)) -> \(called)"
52+
}.value
53+
} catch {
54+
return "WRONG local(\(#function)) thrown(\(error))"
55+
}
56+
}
57+
58+
distributed func callDetachedSelf() async -> String {
59+
do {
60+
return try await Task.detached {
61+
let called = try await self.callTaskSelf_inner() // shouldn't use the distributed thunk!
62+
return "local(\(#function)) -> \(called)"
63+
}.value
64+
} catch {
65+
return "WRONG local(\(#function)) thrown(\(error))"
66+
}
67+
}
68+
4669
// === errors
4770

4871
distributed func helloThrowsImplBoom() throws -> String {
@@ -76,6 +99,21 @@ extension SomeSpecificDistributedActor {
7699
"remote(\(#function))"
77100
}
78101

102+
@_dynamicReplacement(for:_remote_callTaskSelf())
103+
nonisolated func _remote_impl_callTaskSelf() async throws -> String {
104+
"remote(\(#function))"
105+
}
106+
107+
@_dynamicReplacement(for:_remote_callDetachedSelf())
108+
nonisolated func _remote_impl_callDetachedSelf() async throws -> String {
109+
"remote(\(#function))"
110+
}
111+
112+
@_dynamicReplacement(for:_remote_callTaskSelf_inner())
113+
nonisolated func _remote_impl_callTaskSelf_inner() async throws -> String {
114+
"remote(\(#function))"
115+
}
116+
79117
// === errors
80118

81119
@_dynamicReplacement(for:_remote_helloThrowsImplBoom())
@@ -103,9 +141,6 @@ func __isLocalActor(_ actor: AnyObject) -> Bool {
103141
@available(SwiftStdlib 5.5, *)
104142
struct ActorAddress: ActorIdentity {
105143
let address: String
106-
init(parse address : String) {
107-
self.address = address
108-
}
109144
}
110145

111146
@available(SwiftStdlib 5.5, *)
@@ -114,15 +149,15 @@ struct FakeTransport: ActorTransport {
114149
fatalError("not implemented:\(#function)")
115150
}
116151

117-
func resolve<Act>(_ identity: Act.ID, as actorType: Act.Type)
152+
func resolve<Act>(_ identity: AnyActorIdentity, as actorType: Act.Type)
118153
throws -> Act?
119154
where Act: DistributedActor {
120155
return nil
121156
}
122157

123158
func assignIdentity<Act>(_ actorType: Act.Type) -> AnyActorIdentity
124159
where Act: DistributedActor {
125-
.init(ActorAddress(parse: ""))
160+
.init(ActorAddress(address: ""))
126161
}
127162

128163
func actorReady<Act>(_ actor: Act) where Act: DistributedActor {
@@ -151,25 +186,31 @@ func test_remote_invoke(address: ActorAddress, transport: ActorTransport) async
151186
let h4 = try! await actor.hello()
152187
print("\(personality) - hello: \(h4)")
153188

189+
let h5 = try! await actor.callTaskSelf()
190+
print("\(personality) - callTaskSelf: \(h5)")
191+
192+
let h6 = try! await actor.callDetachedSelf()
193+
print("\(personality) - callDetachedSelf: \(h6)")
194+
154195
// error throws
155196
if __isRemoteActor(actor) {
156197
do {
157198
_ = try await actor.helloThrowsTransportBoom()
158-
preconditionFailure("helloThrowsTransportBoom: should have thrown")
199+
print("WRONG: helloThrowsTransportBoom: should have thrown")
159200
} catch {
160201
print("\(personality) - helloThrowsTransportBoom: \(error)")
161202
}
162203
} else {
163204
do {
164205
_ = try await actor.helloThrowsImplBoom()
165-
preconditionFailure("helloThrowsImplBoom: Should have thrown")
206+
print("WRONG: helloThrowsImplBoom: Should have thrown")
166207
} catch {
167208
print("\(personality) - helloThrowsImplBoom: \(error)")
168209
}
169210
}
170211
}
171212

172-
let remote = try! SomeSpecificDistributedActor(resolve: .init(address), using: transport)
213+
let remote = try! SomeSpecificDistributedActor.resolve(.init(address), using: transport)
173214
assert(__isRemoteActor(remote) == true, "should be remote")
174215

175216
let local = SomeSpecificDistributedActor(transport: transport)
@@ -182,6 +223,8 @@ func test_remote_invoke(address: ActorAddress, transport: ActorTransport) async
182223
// CHECK: local - helloAsync: local(helloAsync())
183224
// CHECK: local - helloThrows: local(helloThrows())
184225
// CHECK: local - hello: local(hello())
226+
// CHECK: local - callTaskSelf: local(callTaskSelf()) -> local(callTaskSelf_inner())
227+
// CHECK: local - callDetachedSelf: local(callDetachedSelf()) -> local(callTaskSelf_inner())
185228
// CHECK: local - helloThrowsImplBoom: Boom(whoFailed: "impl")
186229

187230
print("remote isRemote: \(__isRemoteActor(remote))")
@@ -191,6 +234,8 @@ func test_remote_invoke(address: ActorAddress, transport: ActorTransport) async
191234
// CHECK: remote - helloAsync: remote(_remote_impl_helloAsync())
192235
// CHECK: remote - helloThrows: remote(_remote_impl_helloThrows())
193236
// CHECK: remote - hello: remote(_remote_impl_hello())
237+
// CHECK: remote - callTaskSelf: remote(_remote_impl_callTaskSelf())
238+
// CHECK: remote - callDetachedSelf: remote(_remote_impl_callDetachedSelf())
194239
// CHECK: remote - helloThrowsTransportBoom: Boom(whoFailed: "transport")
195240

196241
print(local)
@@ -200,7 +245,7 @@ func test_remote_invoke(address: ActorAddress, transport: ActorTransport) async
200245
@available(SwiftStdlib 5.5, *)
201246
@main struct Main {
202247
static func main() async {
203-
let address = ActorAddress(parse: "")
248+
let address = ActorAddress(address: "")
204249
let transport = FakeTransport()
205250

206251
await test_remote_invoke(address: address, transport: transport)

‎test/Distributed/Runtime/distributed_no_transport_boom.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ struct FakeTransport: ActorTransport {
3636
fatalError("not implemented:\(#function)")
3737
}
3838

39-
func resolve<Act>(_ identity: Act.ID, as actorType: Act.Type)
39+
func resolve<Act>(_ identity: AnyActorIdentity, as actorType: Act.Type)
4040
throws -> Act?
4141
where Act: DistributedActor {
4242
return nil
@@ -65,7 +65,7 @@ func test_remote() async {
6565
let address = ActorAddress(parse: "")
6666
let transport = FakeTransport()
6767

68-
let remote = try! SomeSpecificDistributedActor(resolve: .init(address), using: transport)
68+
let remote = try! SomeSpecificDistributedActor.resolve(.init(address), using: transport)
6969
_ = try! await remote.hello() // let it crash!
7070

7171
// CHECK: SOURCE_DIR/test/Distributed/Runtime/distributed_no_transport_boom.swift:18: Fatal error: Invoked remote placeholder function '_remote_hello' on remote distributed actor of type 'main.SomeSpecificDistributedActor'. Configure an appropriate 'ActorTransport' for this actor to resolve this error (e.g. by depending on some specific transport library).

0 commit comments

Comments
 (0)