From 957a95be63f484e8befa4ee1a91ed19beeb41d07 Mon Sep 17 00:00:00 2001 From: Konrad 'ktoso' Malawski Date: Mon, 7 Apr 2025 19:43:39 +0900 Subject: [PATCH 1/3] [Distributed] Distributed actor usage through protocol with lib-evo must work This corrects how we were dealing with dispatch thunks -- mostly be removing a lot of special casing we did but doesn't seem necessary and instead we correct and emit all the necessary information int TBD. This builds on https://github.com/swiftlang/swift/pull/74935 by further refining how we fixed that issue, and adds more regression tests. It also removes a load of special casing of distributed thunks in library evolution mode, which is great. Resolves and adds regression test for for rdar://145292018 This is also a more proper fix to the previously resolved but in a not-great-way which caused other issues: - resolves rdar://128284016 - resolves rdar://128310903 --- lib/IRGen/GenMeta.cpp | 15 ++--- lib/IRGen/GenProto.cpp | 6 -- lib/IRGen/IRSymbolVisitor.cpp | 8 --- lib/SIL/IR/SILSymbolVisitor.cpp | 1 - ...or_library_evolution_da_protocol_use.swift | 66 +++++++++++++++++++ 5 files changed, 71 insertions(+), 25 deletions(-) create mode 100644 test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift diff --git a/lib/IRGen/GenMeta.cpp b/lib/IRGen/GenMeta.cpp index 5b6c5854c9c11..62facbd61f240 100644 --- a/lib/IRGen/GenMeta.cpp +++ b/lib/IRGen/GenMeta.cpp @@ -1000,8 +1000,7 @@ namespace { // Emit the dispatch thunk. auto shouldEmitDispatchThunk = - (Resilient || IGM.getOptions().WitnessMethodElimination) && - (!func.isDistributed() || !func.isDistributedThunk()); + (Resilient || IGM.getOptions().WitnessMethodElimination); if (shouldEmitDispatchThunk) { IGM.emitDispatchThunk(func); } @@ -1082,14 +1081,10 @@ namespace { // Define the method descriptor. SILDeclRef func(entry.getFunction()); - /// Distributed thunks don't need method descriptors - if (!func.isDistributedThunk()) { - auto *descriptor = - B.getAddrOfCurrentPosition( - IGM.ProtocolRequirementStructTy); - IGM.defineMethodDescriptor(func, Proto, descriptor, - IGM.ProtocolRequirementStructTy); - } + auto *descriptor = + B.getAddrOfCurrentPosition(IGM.ProtocolRequirementStructTy); + IGM.defineMethodDescriptor(func, Proto, descriptor, + IGM.ProtocolRequirementStructTy); } } diff --git a/lib/IRGen/GenProto.cpp b/lib/IRGen/GenProto.cpp index 695beeca36789..f9537053e984c 100644 --- a/lib/IRGen/GenProto.cpp +++ b/lib/IRGen/GenProto.cpp @@ -2348,12 +2348,6 @@ namespace { LinkEntity::forBaseConformanceDescriptor(requirement)); B.addRelativeAddress(baseConformanceDescriptor); } else if (entry.getKind() == SILWitnessTable::Method) { - // distributed thunks don't need resilience - if (entry.getMethodWitness().Requirement.isDistributedThunk()) { - witnesses = witnesses.drop_back(); - continue; - } - // Method descriptor. auto declRef = entry.getMethodWitness().Requirement; auto requirement = diff --git a/lib/IRGen/IRSymbolVisitor.cpp b/lib/IRGen/IRSymbolVisitor.cpp index 7134aa8752b9f..b62d906146aa4 100644 --- a/lib/IRGen/IRSymbolVisitor.cpp +++ b/lib/IRGen/IRSymbolVisitor.cpp @@ -157,14 +157,6 @@ class IRSymbolVisitorImpl : public SILSymbolVisitor { } void addMethodDescriptor(SILDeclRef declRef) override { - if (declRef.isDistributedThunk()) { - auto afd = declRef.getAbstractFunctionDecl(); - auto DC = afd->getDeclContext(); - if (isa(DC)) { - return; - } - } - addLinkEntity(LinkEntity::forMethodDescriptor(declRef)); } diff --git a/lib/SIL/IR/SILSymbolVisitor.cpp b/lib/SIL/IR/SILSymbolVisitor.cpp index be6078d41eade..3da9722583e55 100644 --- a/lib/SIL/IR/SILSymbolVisitor.cpp +++ b/lib/SIL/IR/SILSymbolVisitor.cpp @@ -835,7 +835,6 @@ class SILSymbolVisitorImpl : public ASTVisitor { V.Ctx.getOpts().WitnessMethodElimination} {} void addMethod(SILDeclRef declRef) { - // TODO: alternatively maybe prevent adding distributed thunk here rather than inside those? if (Resilient || WitnessMethodElimination) { Visitor.addDispatchThunk(declRef); Visitor.addMethodDescriptor(declRef); diff --git a/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift b/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift new file mode 100644 index 0000000000000..eafc0d8f87946 --- /dev/null +++ b/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift @@ -0,0 +1,66 @@ +// REQUIRES: VENDOR=apple +// REQUIRES: concurrency +// REQUIRES: distributed + +// RUN: %empty-directory(%t) +// RUN: split-file %s %t + +// RUN: %target-build-swift -Xfrontend -validate-tbd-against-ir=all -enable-library-evolution -target %target-cpu-apple-macosx13.0 -parse-as-library -emit-library -emit-module-path %t/Library.swiftmodule -module-name Library %t/library.swift -o %t/%target-library-name(Library) +// RUN: %target-build-swift -Xfrontend -validate-tbd-against-ir=all -target %target-cpu-apple-macosx13.0 -parse-as-library -lLibrary -module-name main -I %t -L %t %t/main.swift -o %t/a.out + + +// RUN: %target-codesign %t/a.out +// RUN: %target-run %t/a.out + +//--- library.swift +import Distributed + +//public protocol NormalProtocol { +// func NORMAL() async -> Int +//} + +public protocol SimpleProtocol: DistributedActor + where ActorSystem == LocalTestingDistributedActorSystem { + + // nonisolated override var id: ID { get } // comes from DistributedActor + + // Has to have a distributed method to fail + distributed func test() -> Int +} + +//--- main.swift +import Distributed +import Library + +//actor NormalActor: NormalProtocol { +// func NORMAL() async -> Int { 1 } +//} + +public distributed actor SimpleActor: SimpleProtocol { + public distributed func test() -> Int { 1 } +} + +// Passes +public func makeFromPass(_ act: Act) { + print(act.id) +} + +// Fails +public func makeFromFail(_ act: Act) async { + print(act.id) + try! await print(act.test()) +} + +@main +struct TestSwiftFrameworkTests { + static func main() async { + let system = LocalTestingDistributedActorSystem() + + // let norm = NormalActor() + + let simpleActor = SimpleActor(actorSystem: system) +// makeFromPass(simpleActor) + + await makeFromFail(simpleActor) + } +} \ No newline at end of file From 7d0062ed5013172a0be4247d02ee7fb59ddb537c Mon Sep 17 00:00:00 2001 From: Konrad 'ktoso' Malawski Date: Mon, 21 Apr 2025 11:57:55 +0900 Subject: [PATCH 2/3] Guard test to be linux or macOS, surprising build problem on simulator --- .../distributed_actor_library_evolution_da_protocol_use.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift b/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift index eafc0d8f87946..78e4b6de3b577 100644 --- a/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift +++ b/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift @@ -1,6 +1,8 @@ // REQUIRES: VENDOR=apple +// REQUIRES: OS=macosx || OS=linux-gnu // REQUIRES: concurrency // REQUIRES: distributed +// UNSUPPORTED: use_os_stdlib // RUN: %empty-directory(%t) // RUN: split-file %s %t @@ -8,7 +10,6 @@ // RUN: %target-build-swift -Xfrontend -validate-tbd-against-ir=all -enable-library-evolution -target %target-cpu-apple-macosx13.0 -parse-as-library -emit-library -emit-module-path %t/Library.swiftmodule -module-name Library %t/library.swift -o %t/%target-library-name(Library) // RUN: %target-build-swift -Xfrontend -validate-tbd-against-ir=all -target %target-cpu-apple-macosx13.0 -parse-as-library -lLibrary -module-name main -I %t -L %t %t/main.swift -o %t/a.out - // RUN: %target-codesign %t/a.out // RUN: %target-run %t/a.out From d79a78e8985f34debb28a833d3a536c9c09b6864 Mon Sep 17 00:00:00 2001 From: Konrad 'ktoso' Malawski Date: Mon, 21 Apr 2025 12:01:02 +0900 Subject: [PATCH 3/3] pr review followup --- ...or_library_evolution_da_protocol_use.swift | 29 +++++-------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift b/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift index 78e4b6de3b577..e4f65597d2420 100644 --- a/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift +++ b/test/Distributed/Runtime/distributed_actor_library_evolution_da_protocol_use.swift @@ -11,15 +11,11 @@ // RUN: %target-build-swift -Xfrontend -validate-tbd-against-ir=all -target %target-cpu-apple-macosx13.0 -parse-as-library -lLibrary -module-name main -I %t -L %t %t/main.swift -o %t/a.out // RUN: %target-codesign %t/a.out -// RUN: %target-run %t/a.out +// RUN: %target-run %t/a.out | %FileCheck %s //--- library.swift import Distributed -//public protocol NormalProtocol { -// func NORMAL() async -> Int -//} - public protocol SimpleProtocol: DistributedActor where ActorSystem == LocalTestingDistributedActorSystem { @@ -33,23 +29,18 @@ public protocol SimpleProtocol: DistributedActor import Distributed import Library -//actor NormalActor: NormalProtocol { -// func NORMAL() async -> Int { 1 } -//} - public distributed actor SimpleActor: SimpleProtocol { - public distributed func test() -> Int { 1 } -} - -// Passes -public func makeFromPass(_ act: Act) { - print(act.id) + public distributed func test() -> Int { + print("SimpleActor.test") + return 1 + } } -// Fails public func makeFromFail(_ act: Act) async { print(act.id) - try! await print(act.test()) + try! await print("act.test() = \(act.test())") + // CHECK: SimpleActor.test + // CHECK: act.test() = 1 } @main @@ -57,11 +48,7 @@ struct TestSwiftFrameworkTests { static func main() async { let system = LocalTestingDistributedActorSystem() - // let norm = NormalActor() - let simpleActor = SimpleActor(actorSystem: system) -// makeFromPass(simpleActor) - await makeFromFail(simpleActor) } } \ No newline at end of file