Merge pull request swiftlang#58745 from ktoso/wip-init-crash

[Distributed] Enforce strict order of synthesized id and actorSystem properties; avoid wrong offset crashes

Author: ktoso

lib/AST/DistributedDecl.cpp

@@ -472,6 +472,7 @@ bool AbstractFunctionDecl::isDistributedActorSystemRemoteCall(bool isVoidReturn)
   if (!invocationParam->isInOut()) {
     return false;
   }
+
   // --- Check parameter: throwing: Err.Type
   auto thrownTypeParam = params->get(3);
   if (thrownTypeParam->getArgumentName() != C.Id_throwing) {

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,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<VarDecl>(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<FuncDecl>(requirement)) {

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();
 }
@@ -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);

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

@@ -11,7 +11,6 @@
 //===----------------------------------------------------------------------===//
 
 import Swift
-import _Concurrency
 
 #if canImport(Darwin)
 import Darwin
@@ -27,33 +26,25 @@ import WinSDK
 /// for learning about `distributed actor` isolation, as well as early
 /// prototyping stages of development where a real system is not necessary yet.
 @available(SwiftStdlib 5.7, *)
-public struct LocalTestingDistributedActorSystem: DistributedActorSystem, @unchecked Sendable {
+public final class LocalTestingDistributedActorSystem: DistributedActorSystem, @unchecked Sendable {
   public typealias ActorID = LocalTestingActorAddress
   public typealias ResultHandler = LocalTestingInvocationResultHandler
   public typealias InvocationEncoder = LocalTestingInvocationEncoder
   public typealias InvocationDecoder = LocalTestingInvocationDecoder
-  public typealias SerializationRequirement = any Codable
-
-
-  @usableFromInline
-  final class _Storage {
+  public typealias SerializationRequirement = Codable
 
-    var activeActors: [ActorID: any DistributedActor] = [:]
-    let activeActorsLock = _Lock()
+  private var activeActors: [ActorID: any DistributedActor] = [:]
+  private let activeActorsLock = _Lock()
 
-    var assignedIDs: Set<ActorID> = []
-    let assignedIDsLock = _Lock()
-  }
-  let storage: _Storage
-  var idProvider: ActorIDProvider = ActorIDProvider()
+  private var idProvider: ActorIDProvider = ActorIDProvider()
+  private var assignedIDs: Set<ActorID> = []
+  private let assignedIDsLock = _Lock()
 
-  public init() {
-    storage = .init()
-  }
+  public init() {}
 
   public func resolve<Act>(id: ActorID, as actorType: Act.Type)
     throws -> Act? where Act: DistributedActor, Act.ID == ActorID {
-    guard let anyActor = storage.activeActorsLock.withLock({ self.storage.activeActors[id] }) else {
+    guard let anyActor = self.activeActorsLock.withLock({ self.activeActors[id] }) else {
       throw LocalTestingDistributedActorSystemError(message: "Unable to locate id '\(id)' locally")
     }
     guard let actor = anyActor as? Act else {
@@ -65,23 +56,25 @@ public struct LocalTestingDistributedActorSystem: DistributedActorSystem, @unche
   public func assignID<Act>(_ actorType: Act.Type) -> ActorID
     where Act: DistributedActor, Act.ID == ActorID {
     let id = self.idProvider.next()
-    storage.assignedIDsLock.withLock {
-      self.storage.assignedIDs.insert(id)
+    self.assignedIDsLock.withLock {
+      self.assignedIDs.insert(id)
     }
     return id
   }
 
   public func actorReady<Act>(_ actor: Act)
     where Act: DistributedActor, Act.ID == ActorID {
-    guard storage.assignedIDsLock.withLock({ self.storage.assignedIDs.contains(actor.id) }) else {
+    guard self.assignedIDsLock.withLock({ self.assignedIDs.contains(actor.id) }) else {
       fatalError("Attempted to mark an unknown actor '\(actor.id)' ready")
     }
-    self.storage.activeActors[actor.id] = actor
+    self.activeActorsLock.withLock {
+      self.activeActors[actor.id] = actor
+    }
   }
 
   public func resignID(_ id: ActorID) {
-    storage.activeActorsLock.withLock {
-      self.storage.activeActors.removeValue(forKey: id)
+    self.activeActorsLock.withLock {
+      self.activeActors.removeValue(forKey: id)
     }
   }
 
@@ -99,7 +92,7 @@ public struct LocalTestingDistributedActorSystem: DistributedActorSystem, @unche
     where Act: DistributedActor,
           Act.ID == ActorID,
           Err: Error,
-          Res: Codable {
+          Res: SerializationRequirement {
     fatalError("Attempted to make remote call to \(target) on actor \(actor) using a local-only actor system")
   }
 
@@ -115,40 +108,51 @@ public struct LocalTestingDistributedActorSystem: DistributedActorSystem, @unche
     fatalError("Attempted to make remote call to \(target) on actor \(actor) using  a local-only actor system")
   }
 
-  @usableFromInline
-  final class ActorIDProvider {
+  private struct ActorIDProvider {
     private var counter: Int = 0
     private let counterLock = _Lock()
 
-    @usableFromInline
     init() {}
 
-    func next() -> LocalTestingActorAddress {
+    mutating func next() -> LocalTestingActorAddress {
       let id: Int = self.counterLock.withLock {
         self.counter += 1
         return self.counter
       }
-      return LocalTestingActorAddress(id)
+      return LocalTestingActorAddress(parse: "\(id)")
     }
   }
 }
 
 @available(SwiftStdlib 5.7, *)
-public struct LocalTestingActorAddress: Hashable, Sendable, Codable {
-  public let uuid: String
+@available(*, deprecated, renamed: "LocalTestingActorID")
+public typealias LocalTestingActorAddress = LocalTestingActorID
 
-  public init(_ uuid: Int) {
-    self.uuid = "\(uuid)"
+@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.uuid = 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.uuid)
+    try container.encode(self.id)
   }
 }
 
@@ -228,7 +232,7 @@ public struct LocalTestingDistributedActorSystemError: DistributedActorSystemErr
 // === lock ----------------------------------------------------------------
 
 @available(SwiftStdlib 5.7, *)
-internal class _Lock {
+fileprivate class _Lock {
   #if os(iOS) || os(macOS) || os(tvOS) || os(watchOS)
   private let underlying: UnsafeMutablePointer<os_unfair_lock>
   #elseif os(Windows)
@@ -277,6 +281,7 @@ internal class _Lock {
     #endif
   }
 
+
   @discardableResult
   func withLock<T>(_ body: () -> T) -> T {
     #if os(iOS) || os(macOS) || os(tvOS) || os(watchOS)

test/Distributed/Runtime/distributed_actor_init_local.swift

@@ -8,8 +8,6 @@
 // UNSUPPORTED: use_os_stdlib
 // UNSUPPORTED: back_deployment_runtime
 
-// REQUIRES: rdar92910719
-
 import Distributed
 
 enum MyError: Error {
@@ -92,15 +90,6 @@ distributed actor MaybeAfterAssign {
   }
 }
 
-distributed actor LocalTestingSystemDA {
-  typealias ActorSystem = LocalTestingDistributedActorSystem
-  var x: Int
-  init() {
-    actorSystem = .init()
-    x = 100
-  }
-}
-
 distributed actor LocalTestingDA_Int {
   typealias ActorSystem = LocalTestingDistributedActorSystem
   var int: Int
@@ -111,7 +100,6 @@ distributed actor LocalTestingDA_Int {
   }
 }
 
-
 // ==== Fake Transport ---------------------------------------------------------
 
 struct ActorAddress: Sendable, Hashable, Codable {
@@ -286,7 +274,7 @@ func test() async {
 
   let localDA = LocalTestingDA_Int()
   print("localDA = \(localDA.id)")
-  // CHECK: localDA = LocalTestingActorAddress(uuid: "1")
+  // CHECK: localDA = LocalTestingActorID(id: "1")
 
   // the following tests fail to initialize the actor's identity.
   print("-- start of no-assign tests --")
@@ -297,6 +285,7 @@ func test() async {
   // CHECK-NOT: assign
   // CHECK: -- end of no-assign tests --
 
+
   // resigns that come out of the deinits:
 
   // CHECK-DAG: resign id:ActorAddress(address: "[[ID1]]")

test/Distributed/distributed_actor_layout.swift

@@ -12,6 +12,8 @@ import FakeDistributedActorSystems
 @available(SwiftStdlib 5.7, *)
 typealias DefaultDistributedActorSystem = FakeActorSystem
 
+class MyClass { }
+
 // Ensure that the actor layout is (metadata pointer, default actor, id, system,
 // <user fields>)
 
@@ -21,10 +23,10 @@ protocol HasActorSystem {
 
 extension MyActor: HasActorSystem { }
 
-// CHECK: %T27distributed_actor_accessors7MyActorC = type <{ %swift.refcounted, %swift.defaultactor, %T27FakeDistributedActorSystems0C7AddressV, %T27FakeDistributedActorSystems0aC6SystemV, %TSS }>
+// CHECK: %T27distributed_actor_accessors7MyActorC = type <{ %swift.refcounted, %swift.defaultactor, %T27FakeDistributedActorSystems0C7AddressV, %T27FakeDistributedActorSystems0aC6SystemV, %T27distributed_actor_accessors7MyClassC* }>
 @available(SwiftStdlib 5.7, *)
 public distributed actor MyActor {
-  var field: String = ""
+  var field: MyClass = MyClass()
 
   init(actorSystem: FakeActorSystem) {
     self.actorSystem = actorSystem