From fee52c63aed6589eacb484d4a19902cfe30e513f Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Sun, 8 May 2022 17:40:51 +0900 Subject: [PATCH 1/5] [Distributed] Force the order of properties in AST missing type decl --- lib/Sema/CodeSynthesisDistributedActor.cpp | 14 ++++-- .../DerivedConformanceDistributedActor.cpp | 47 ++++++------------- lib/Sema/DerivedConformances.cpp | 16 +++++-- lib/Sema/DerivedConformances.h | 4 ++ .../LocalTestingDistributedActorSystem.swift | 31 ++++++++---- .../distributed_actor_init_local.swift | 19 +++++++- test/IRGen/distributed_actor.swift | 2 +- 7 files changed, 82 insertions(+), 51 deletions(-) diff --git a/lib/Sema/CodeSynthesisDistributedActor.cpp b/lib/Sema/CodeSynthesisDistributedActor.cpp index ec5bd3b3d7041..861908e603c66 100644 --- a/lib/Sema/CodeSynthesisDistributedActor.cpp +++ b/lib/Sema/CodeSynthesisDistributedActor.cpp @@ -117,9 +117,17 @@ static VarDecl *addImplicitDistributedActorIDProperty( propDecl->getAttrs().add( new (C) CompilerInitializedAttr(/*IsImplicit=*/true)); - nominal->addMember(propDecl); - nominal->addMember(pbDecl); - + // IMPORTANT: The `id` MUST be the first field of any distributed actor, + // because when we allocate remote proxy instances, we don't allocate memory + // for anything except the first two fields: id and actorSystem, so they + // MUST be those fields. + // + // Their specific order also matters, because it is enforced this way in IRGen + // and how we emit them in AST MUST match what IRGen expects or cross-module + // things could be using wrong offsets and manifest as reading trash memory on + // id or system accesses. + nominal->addMember(propDecl, /*hint=*/nullptr, /*insertAtHead=*/true); + nominal->addMember(pbDecl, /*hint=*/nullptr, /*insertAtHead=*/true); return propDecl; } diff --git a/lib/Sema/DerivedConformanceDistributedActor.cpp b/lib/Sema/DerivedConformanceDistributedActor.cpp index e87bcc527b029..318dac95c355f 100644 --- a/lib/Sema/DerivedConformanceDistributedActor.cpp +++ b/lib/Sema/DerivedConformanceDistributedActor.cpp @@ -426,32 +426,6 @@ static FuncDecl *deriveDistributedActorSystem_invokeHandlerOnReturn( /******************************* PROPERTIES ***********************************/ /******************************************************************************/ -// TODO(distributed): make use of this after all, but FORCE it? -static ValueDecl *deriveDistributedActor_id(DerivedConformance &derived) { - assert(derived.Nominal->isDistributedActor()); - auto &C = derived.Context; - - // ``` - // nonisolated - // let id: Self.ID // Self.ActorSystem.ActorID - // ``` - auto propertyType = getDistributedActorIDType(derived.Nominal); - - VarDecl *propDecl; - PatternBindingDecl *pbDecl; - std::tie(propDecl, pbDecl) = derived.declareDerivedProperty( - DerivedConformance::SynthesizedIntroducer::Let, C.Id_id, propertyType, - propertyType, - /*isStatic=*/false, /*isFinal=*/true); - - // mark as nonisolated, allowing access to it from everywhere - propDecl->getAttrs().add( - new (C) NonisolatedAttr(/*IsImplicit=*/true)); - - derived.addMembersToConformanceContext({ propDecl, pbDecl }); - return propDecl; -} - static ValueDecl *deriveDistributedActor_actorSystem( DerivedConformance &derived) { auto &C = derived.Context; @@ -460,8 +434,7 @@ static ValueDecl *deriveDistributedActor_actorSystem( assert(classDecl && derived.Nominal->isDistributedActor()); // ``` - // nonisolated - // let actorSystem: ActorSystem + // nonisolated let actorSystem: ActorSystem // ``` // (no need for @actorIndependent because it is an immutable let) auto propertyType = getDistributedActorSystemType(classDecl); @@ -477,7 +450,14 @@ static ValueDecl *deriveDistributedActor_actorSystem( propDecl->getAttrs().add( new (C) NonisolatedAttr(/*IsImplicit=*/true)); - derived.addMembersToConformanceContext({ propDecl, pbDecl }); + // IMPORTANT: `id` MUST be the first field of a distributed actor, and + // `actorSystem` MUST be the second field, because for a remote instance + // we don't allocate memory after those two fields, so their order is very + // important. The `hint` below makes sure the system is inserted right after. + auto id = derived.Nominal->getDistributedActorIDProperty(); + derived.addMemberToConformanceContext(pbDecl, /*hint=*/id); + derived.addMemberToConformanceContext(propDecl, /*hint=*/id); + return propDecl; } @@ -571,11 +551,14 @@ deriveDistributedActorType_SerializationRequirement( ValueDecl *DerivedConformance::deriveDistributedActor(ValueDecl *requirement) { if (auto var = dyn_cast(requirement)) { - if (var->getName() == Context.Id_id) - return deriveDistributedActor_id(*this); - if (var->getName() == Context.Id_actorSystem) return deriveDistributedActor_actorSystem(*this); + + if (var->getName() == Context.Id_id) + llvm_unreachable("DistributedActor.id MUST be synthesized earlier, " + "because it is forced by the Identifiable conformance. " + "If we attempted to do synthesis here, the earlier phase " + "failed and something is wrong: please report a bug."); } if (auto func = dyn_cast(requirement)) { diff --git a/lib/Sema/DerivedConformances.cpp b/lib/Sema/DerivedConformances.cpp index f7dd468863ac4..0ccaa2a253dd7 100644 --- a/lib/Sema/DerivedConformances.cpp +++ b/lib/Sema/DerivedConformances.cpp @@ -51,6 +51,18 @@ void DerivedConformance::addMembersToConformanceContext( IDC->addMember(child); } +void DerivedConformance::addMemberToConformanceContext( + Decl *member, Decl *hint) { + auto IDC = cast(ConformanceDecl); + IDC->addMember(member, hint, /*insertAtHead=*/false); +} + +void DerivedConformance::addMemberToConformanceContext( + Decl *member, bool insertAtHead) { + auto IDC = cast(ConformanceDecl); + IDC->addMember(member, /*hint=*/nullptr, insertAtHead); +} + Type DerivedConformance::getProtocolType() const { return Protocol->getDeclaredInterfaceType(); } @@ -325,10 +337,6 @@ ValueDecl *DerivedConformance::getDerivableRequirement(NominalTypeDecl *nominal, if (name.isSimpleName(ctx.Id_unownedExecutor)) return getRequirement(KnownProtocolKind::Actor); - // DistributedActor.id - if(name.isSimpleName(ctx.Id_id)) - return getRequirement(KnownProtocolKind::DistributedActor); - // DistributedActor.actorSystem if(name.isSimpleName(ctx.Id_actorSystem)) return getRequirement(KnownProtocolKind::DistributedActor); diff --git a/lib/Sema/DerivedConformances.h b/lib/Sema/DerivedConformances.h index 281b365391912..f1d2fe1262c4c 100644 --- a/lib/Sema/DerivedConformances.h +++ b/lib/Sema/DerivedConformances.h @@ -70,6 +70,10 @@ class DerivedConformance { /// Add \c children as members of the context that declares the conformance. void addMembersToConformanceContext(ArrayRef children); + /// Add \c member right after the \c hint member which may be the tail + void addMemberToConformanceContext(Decl *member, Decl* hint); + /// Add \c member in front of any other existing members + void addMemberToConformanceContext(Decl *member, bool insertAtHead); /// Get the declared type of the protocol that this is conformance is for. Type getProtocolType() const; diff --git a/stdlib/public/Distributed/LocalTestingDistributedActorSystem.swift b/stdlib/public/Distributed/LocalTestingDistributedActorSystem.swift index 90c108c1f38f5..f494351689c12 100644 --- a/stdlib/public/Distributed/LocalTestingDistributedActorSystem.swift +++ b/stdlib/public/Distributed/LocalTestingDistributedActorSystem.swift @@ -27,7 +27,7 @@ import WinSDK /// prototyping stages of development where a real system is not necessary yet. @available(SwiftStdlib 5.7, *) public final class LocalTestingDistributedActorSystem: DistributedActorSystem, @unchecked Sendable { - public typealias ActorID = LocalTestingActorAddress + public typealias ActorID = LocalTestingActorID public typealias ResultHandler = LocalTestingInvocationResultHandler public typealias InvocationEncoder = LocalTestingInvocationEncoder public typealias InvocationDecoder = LocalTestingInvocationDecoder @@ -115,32 +115,45 @@ public final class LocalTestingDistributedActorSystem: DistributedActorSystem, @ init() {} - mutating func next() -> LocalTestingActorAddress { + mutating func next() -> LocalTestingActorID { let id: Int = self.counterLock.withLock { self.counter += 1 return self.counter } - return LocalTestingActorAddress(parse: "\(id)") + return LocalTestingActorID(id: "\(id)") } } } @available(SwiftStdlib 5.7, *) -public struct LocalTestingActorAddress: Hashable, Sendable, Codable { - public let address: String +@available(*, deprecated, renamed: "LocalTestingActorID") +public typealias LocalTestingActorAddress = LocalTestingActorID - public init(parse address: String) { - self.address = address +@available(SwiftStdlib 5.7, *) +public struct LocalTestingActorID: Hashable, Sendable, Codable { + @available(*, deprecated, renamed: "id") + public var address: String { + self.id + } + public let id: String + + @available(*, deprecated, renamed: "init(id:)") + public init(parse id: String) { + self.id = id + } + + public init(id: String) { + self.id = id } public init(from decoder: Decoder) throws { let container = try decoder.singleValueContainer() - self.address = try container.decode(String.self) + self.id = try container.decode(String.self) } public func encode(to encoder: Encoder) throws { var container = encoder.singleValueContainer() - try container.encode(self.address) + try container.encode(self.id) } } diff --git a/test/Distributed/Runtime/distributed_actor_init_local.swift b/test/Distributed/Runtime/distributed_actor_init_local.swift index ce72b2dbb2479..f90cb6890ec70 100644 --- a/test/Distributed/Runtime/distributed_actor_init_local.swift +++ b/test/Distributed/Runtime/distributed_actor_init_local.swift @@ -21,8 +21,8 @@ distributed actor PickATransport1 { } distributed actor PickATransport2 { - init(other: Int, thesystem: FakeActorSystem) async { - self.actorSystem = thesystem + init(other: Int, theSystem: FakeActorSystem) async { + self.actorSystem = theSystem } } @@ -90,6 +90,17 @@ distributed actor MaybeAfterAssign { } } +distributed actor LocalTestingDA_Int { + typealias ActorSystem = LocalTestingDistributedActorSystem + var int: Int + init() { + actorSystem = .init() + int = 12 + // CRASH + } +} + + // ==== Fake Transport --------------------------------------------------------- struct ActorAddress: Sendable, Hashable, Codable { @@ -262,6 +273,10 @@ func test() async { // CHECK: assign type:MaybeAfterAssign, id:ActorAddress(address: "[[ID10:.*]]") // CHECK-NEXT: ready actor:main.MaybeAfterAssign, id:ActorAddress(address: "[[ID10]]") + let localDA = LocalTestingDA_Int() + print("localDA = \(localDA.id)") + // CHECK: localDA = LocalTestingActorID(id: "1") + // the following tests fail to initialize the actor's identity. print("-- start of no-assign tests --") test.append(MaybeSystem(nil)) diff --git a/test/IRGen/distributed_actor.swift b/test/IRGen/distributed_actor.swift index 3df58d2bc7126..57a5653551f9b 100644 --- a/test/IRGen/distributed_actor.swift +++ b/test/IRGen/distributed_actor.swift @@ -6,7 +6,7 @@ import Distributed // Type descriptor. -// CHECK-LABEL: @"$s17distributed_actor7MyActorC2id11Distributed012LocalTestingD7AddressVvpWvd" +// CHECK-LABEL: @"$s17distributed_actor7MyActorC2id11Distributed012LocalTestingD2IDVvpWvd" @available(SwiftStdlib 5.6, *) public distributed actor MyActor { public typealias ActorSystem = LocalTestingDistributedActorSystem From c9cd2d3d27dee0566c7c6bb92d06149d26754e3f Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Mon, 9 May 2022 18:30:12 +0900 Subject: [PATCH 2/5] [Distributed] restore id synthesis in DerivedConformance infra for multi module cases --- .../DerivedConformanceDistributedActor.cpp | 49 +++++++++++++++---- lib/Sema/DerivedConformances.cpp | 4 ++ test/Distributed/Inputs/EchoActor.swift | 2 +- .../distributed_actor_in_other_module.swift | 1 - 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/lib/Sema/DerivedConformanceDistributedActor.cpp b/lib/Sema/DerivedConformanceDistributedActor.cpp index 318dac95c355f..9e3635f49fa9b 100644 --- a/lib/Sema/DerivedConformanceDistributedActor.cpp +++ b/lib/Sema/DerivedConformanceDistributedActor.cpp @@ -426,6 +426,32 @@ static FuncDecl *deriveDistributedActorSystem_invokeHandlerOnReturn( /******************************* PROPERTIES ***********************************/ /******************************************************************************/ +// TODO(distributed): make use of this after all, but FORCE it? +static ValueDecl *deriveDistributedActor_id(DerivedConformance &derived) { + assert(derived.Nominal->isDistributedActor()); + auto &C = derived.Context; + + // ``` + // nonisolated let id: Self.ID // Self.ActorSystem.ActorID + // ``` + auto propertyType = getDistributedActorIDType(derived.Nominal); + + VarDecl *propDecl; + PatternBindingDecl *pbDecl; + std::tie(propDecl, pbDecl) = derived.declareDerivedProperty( + DerivedConformance::SynthesizedIntroducer::Let, C.Id_id, propertyType, + propertyType, + /*isStatic=*/false, /*isFinal=*/true); + + // mark as nonisolated, allowing access to it from everywhere + propDecl->getAttrs().add( + new (C) NonisolatedAttr(/*IsImplicit=*/true)); + + derived.addMemberToConformanceContext(pbDecl, /*insertAtHead=*/true); + derived.addMemberToConformanceContext(propDecl, /*insertAtHead=*/true); + return propDecl; +} + static ValueDecl *deriveDistributedActor_actorSystem( DerivedConformance &derived) { auto &C = derived.Context; @@ -454,9 +480,17 @@ static ValueDecl *deriveDistributedActor_actorSystem( // `actorSystem` MUST be the second field, because for a remote instance // we don't allocate memory after those two fields, so their order is very // important. The `hint` below makes sure the system is inserted right after. - auto id = derived.Nominal->getDistributedActorIDProperty(); - derived.addMemberToConformanceContext(pbDecl, /*hint=*/id); - derived.addMemberToConformanceContext(propDecl, /*hint=*/id); + if (auto id = derived.Nominal->getDistributedActorIDProperty()) { + derived.addMemberToConformanceContext(pbDecl, /*hint=*/id); + derived.addMemberToConformanceContext(propDecl, /*hint=*/id); + } else { + // it will be synthesized next, and will insert at head, + // so in order for system to be SECOND (as it must be), + // we'll insert at head right now and as id gets synthesized we'll get + // the correct order: id, actorSystem. + derived.addMemberToConformanceContext(pbDecl, /*insertAtHead==*/true); + derived.addMemberToConformanceContext(propDecl, /*insertAtHead=*/true); + } return propDecl; } @@ -551,14 +585,11 @@ deriveDistributedActorType_SerializationRequirement( ValueDecl *DerivedConformance::deriveDistributedActor(ValueDecl *requirement) { if (auto var = dyn_cast(requirement)) { + if (var->getName() == Context.Id_id) + return deriveDistributedActor_id(*this); + if (var->getName() == Context.Id_actorSystem) return deriveDistributedActor_actorSystem(*this); - - if (var->getName() == Context.Id_id) - llvm_unreachable("DistributedActor.id MUST be synthesized earlier, " - "because it is forced by the Identifiable conformance. " - "If we attempted to do synthesis here, the earlier phase " - "failed and something is wrong: please report a bug."); } if (auto func = dyn_cast(requirement)) { diff --git a/lib/Sema/DerivedConformances.cpp b/lib/Sema/DerivedConformances.cpp index 0ccaa2a253dd7..34dfbf5daadc1 100644 --- a/lib/Sema/DerivedConformances.cpp +++ b/lib/Sema/DerivedConformances.cpp @@ -337,6 +337,10 @@ ValueDecl *DerivedConformance::getDerivableRequirement(NominalTypeDecl *nominal, if (name.isSimpleName(ctx.Id_unownedExecutor)) return getRequirement(KnownProtocolKind::Actor); + // DistributedActor.id + if(name.isSimpleName(ctx.Id_id)) + return getRequirement(KnownProtocolKind::DistributedActor); + // DistributedActor.actorSystem if(name.isSimpleName(ctx.Id_actorSystem)) return getRequirement(KnownProtocolKind::DistributedActor); diff --git a/test/Distributed/Inputs/EchoActor.swift b/test/Distributed/Inputs/EchoActor.swift index 6cb39335f28b5..f1a3621d96c89 100644 --- a/test/Distributed/Inputs/EchoActor.swift +++ b/test/Distributed/Inputs/EchoActor.swift @@ -14,7 +14,7 @@ import Distributed -distributed actor Echo /* in the mirror */{ +distributed actor Echo /* in the mirror */ { typealias ActorSystem = LocalTestingDistributedActorSystem distributed func echo(_ input: String) -> String { diff --git a/test/Distributed/Runtime/distributed_actor_in_other_module.swift b/test/Distributed/Runtime/distributed_actor_in_other_module.swift index 0d4889fb320b6..bf00654b71a70 100644 --- a/test/Distributed/Runtime/distributed_actor_in_other_module.swift +++ b/test/Distributed/Runtime/distributed_actor_in_other_module.swift @@ -8,7 +8,6 @@ // REQUIRES: concurrency // REQUIRES: distributed - // UNSUPPORTED: use_os_stdlib // UNSUPPORTED: back_deployment_runtime From de8486adf2170b15d5a972c771da9820d0899a5f Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Mon, 9 May 2022 19:59:35 +0900 Subject: [PATCH 3/5] [Distributed] Disable test on watchos as 5.7 seems to have some missing cherry-picks --- test/Distributed/Runtime/distributed_actor_init_local.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/Distributed/Runtime/distributed_actor_init_local.swift b/test/Distributed/Runtime/distributed_actor_init_local.swift index f90cb6890ec70..12291c2bcdbdf 100644 --- a/test/Distributed/Runtime/distributed_actor_init_local.swift +++ b/test/Distributed/Runtime/distributed_actor_init_local.swift @@ -8,6 +8,10 @@ // UNSUPPORTED: use_os_stdlib // UNSUPPORTED: back_deployment_runtime +// FIXME(distributed): 5.7 branches seem to be missing something; as `main + 32bit watch` does not crash on DA usage with the local testing actor system, but 5.7 does. +// rdar://92952551 +// UNSUPPORTED: OS=watchos + import Distributed enum MyError: Error { From 66d499ac6cb5432fdb5c24f943c3617ecdf73594 Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Mon, 9 May 2022 22:23:51 +0900 Subject: [PATCH 4/5] fix typo --- test/Distributed/Runtime/distributed_actor_init_local.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Distributed/Runtime/distributed_actor_init_local.swift b/test/Distributed/Runtime/distributed_actor_init_local.swift index 12291c2bcdbdf..184caaeb47740 100644 --- a/test/Distributed/Runtime/distributed_actor_init_local.swift +++ b/test/Distributed/Runtime/distributed_actor_init_local.swift @@ -255,7 +255,7 @@ func test() async { // CHECK-NOT: ready // CHECK: resign id:ActorAddress(address: "[[ID5]]") - test.append(await PickATransport2(other: 1, thesystem: system)) + test.append(await PickATransport2(other: 1, theSystem: system)) // CHECK: assign type:PickATransport2, id:ActorAddress(address: "[[ID6:.*]]") // CHECK: ready actor:main.PickATransport2, id:ActorAddress(address: "[[ID6]]") From 26cef202087c22a65465044df26018ec24ec68c2 Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Tue, 10 May 2022 10:11:35 +0900 Subject: [PATCH 5/5] [Distributed] review feedback: avoid un-necessary handling null id; it will always be synthesized or present already --- .../DerivedConformanceDistributedActor.cpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/lib/Sema/DerivedConformanceDistributedActor.cpp b/lib/Sema/DerivedConformanceDistributedActor.cpp index 9e3635f49fa9b..610aa32cd17ef 100644 --- a/lib/Sema/DerivedConformanceDistributedActor.cpp +++ b/lib/Sema/DerivedConformanceDistributedActor.cpp @@ -426,7 +426,6 @@ static FuncDecl *deriveDistributedActorSystem_invokeHandlerOnReturn( /******************************* PROPERTIES ***********************************/ /******************************************************************************/ -// TODO(distributed): make use of this after all, but FORCE it? static ValueDecl *deriveDistributedActor_id(DerivedConformance &derived) { assert(derived.Nominal->isDistributedActor()); auto &C = derived.Context; @@ -480,17 +479,12 @@ static ValueDecl *deriveDistributedActor_actorSystem( // `actorSystem` MUST be the second field, because for a remote instance // we don't allocate memory after those two fields, so their order is very // important. The `hint` below makes sure the system is inserted right after. - if (auto id = derived.Nominal->getDistributedActorIDProperty()) { - derived.addMemberToConformanceContext(pbDecl, /*hint=*/id); - derived.addMemberToConformanceContext(propDecl, /*hint=*/id); - } else { - // it will be synthesized next, and will insert at head, - // so in order for system to be SECOND (as it must be), - // we'll insert at head right now and as id gets synthesized we'll get - // the correct order: id, actorSystem. - derived.addMemberToConformanceContext(pbDecl, /*insertAtHead==*/true); - derived.addMemberToConformanceContext(propDecl, /*insertAtHead=*/true); - } + auto id = derived.Nominal->getDistributedActorIDProperty(); + assert(id && "id must be synthesized first, so it is the first field of any " + "distributed actor (followed by actorSystem)"); + + derived.addMemberToConformanceContext(pbDecl, /*hint=*/id); + derived.addMemberToConformanceContext(propDecl, /*hint=*/id); return propDecl; }