Merge pull request #58747 from ktoso/pick-wip-init-crash-fix

🍒[5.7][Distributed] Force the order of properties in AST

Author: swift-ci

lib/SILGen/SILGenDistributed.cpp

@@ -64,17 +64,12 @@ static void initializeProperty(SILGenFunction &SGF, SILLocation loc,
     SGF.B.createCopyAddr(loc, value, fieldAddr, IsNotTake, IsInitialization);
   } else {
     if (value->getType().isAddress()) {
-      value = SGF.B.createTrivialLoadOr(
-          loc, value, LoadOwnershipQualifier::Take);
+      SGF.emitSemanticLoadInto(loc, value, SGF.F.getTypeLowering(value->getType()),
+          fieldAddr, SGF.getTypeLowering(loweredType), IsTake, IsInitialization);
     } else {
       value = SGF.B.emitCopyValueOperation(loc, value);
-    }
-
-    SGF.B.emitStoreValueOperation(
+      SGF.B.emitStoreValueOperation(
         loc, value, fieldAddr, StoreOwnershipQualifier::Init);
-
-    if (value->getType().isAddress()) {
-      SGF.B.createDestroyAddr(loc, value);
     }
   }
 }

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;
 }
 

lib/Sema/DerivedConformanceDistributedActor.cpp

@@ -426,14 +426,12 @@ 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
+  // nonisolated let id: Self.ID // Self.ActorSystem.ActorID
   // ```
   auto propertyType = getDistributedActorIDType(derived.Nominal);
 
@@ -448,7 +446,8 @@ static ValueDecl *deriveDistributedActor_id(DerivedConformance &derived) {
   propDecl->getAttrs().add(
       new (C) NonisolatedAttr(/*IsImplicit=*/true));
 
-  derived.addMembersToConformanceContext({ propDecl, pbDecl });
+  derived.addMemberToConformanceContext(pbDecl, /*insertAtHead=*/true);
+  derived.addMemberToConformanceContext(propDecl, /*insertAtHead=*/true);
   return propDecl;
 }
 
@@ -460,8 +459,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 +475,17 @@ 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();
+  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;
 }
 

lib/Sema/DerivedConformances.cpp

@@ -51,6 +51,18 @@ void DerivedConformance::addMembersToConformanceContext(
     IDC->addMember(child);
 }
 
+void DerivedConformance::addMemberToConformanceContext(
+    Decl *member, Decl *hint) {
+  auto IDC = cast<IterableDeclContext>(ConformanceDecl);
+  IDC->addMember(member, hint, /*insertAtHead=*/false);
+}
+
+void DerivedConformance::addMemberToConformanceContext(
+    Decl *member, bool insertAtHead) {
+  auto IDC = cast<IterableDeclContext>(ConformanceDecl);
+  IDC->addMember(member, /*hint=*/nullptr, insertAtHead);
+}
+
 Type DerivedConformance::getProtocolType() const {
   return Protocol->getDeclaredInterfaceType();
 }

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<Decl *> 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;

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)
   }
 }
 

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
 

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 {
@@ -21,8 +25,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 +94,15 @@ distributed actor MaybeAfterAssign {
   }
 }
 
+distributed actor LocalTestingDA_Int {
+  typealias ActorSystem = LocalTestingDistributedActorSystem
+  var int: Int
+  init() {
+    actorSystem = .init()
+    int = 12
+  }
+}
+
 // ==== Fake Transport ---------------------------------------------------------
 
 struct ActorAddress: Sendable, Hashable, Codable {
@@ -240,7 +253,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]]")
 
@@ -262,6 +275,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))

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