Merge pull request #60050 from ktoso/wip-deinit-test-da

[Distributed] prevent remote distributed actor's from running deinit bodies

Author: swift-ci

lib/SILGen/SILGenDestructor.cpp

@@ -31,15 +31,60 @@ void SILGenFunction::emitDestroyingDestructor(DestructorDecl *dd) {
     Loc.markAutoGenerated();
 
   auto cd = cast<ClassDecl>(dd->getDeclContext());
+  auto &C = cd->getASTContext();
   SILValue selfValue = emitSelfDecl(dd->getImplicitSelfDecl());
 
   // Create a basic block to jump to for the implicit destruction behavior
   // of releasing the elements and calling the superclass destructor.
   // We won't actually emit the block until we finish with the destructor body.
   prepareEpilog(None, false, CleanupLocation(Loc));
 
+  auto cleanupLoc = CleanupLocation(Loc);
+
+  SILBasicBlock *deinitBodyBB = nullptr;
+  SILBasicBlock *finishBB = nullptr;
+  if (cd->isDistributedActor()) {
+    auto remoteBB = createBasicBlock("remoteActorDeinitBB");
+    finishBB = createBasicBlock("finishDeinitBB");
+    deinitBodyBB = createBasicBlock("deinitBodyBB");
+
+    // FIXME: what should the type of management be for this?
+    auto managedSelf = ManagedValue::forBorrowedRValue(selfValue);
+
+    auto selfTy = F.mapTypeIntoContext(cd->getDeclaredInterfaceType());
+    emitDistributedIfRemoteBranch(
+        SILLocation(Loc),
+        managedSelf, selfTy,
+        /*if remote=*/remoteBB,
+        /*if local=*/deinitBodyBB);
+
+    {
+      B.emitBlock(remoteBB);
+
+      // Note that we do NOT execute user-declared the deinit body.
+      // They would be free to access state which does not exist in a remote DA
+
+      // we are a remote instance,
+      // the only properties we can destroy are the id and system properties.
+      for (VarDecl *vd : cd->getStoredProperties()) {
+        if (getActorIsolation(vd) == ActorIsolation::ActorInstance)
+          continue;
+
+        // Just to double-check, we only want to destroy `id` and `actorSystem`
+        if (vd->getBaseIdentifier() == C.Id_id ||
+            vd->getBaseIdentifier() == C.Id_actorSystem) {
+          destroyClassMember(cleanupLoc, managedSelf, vd);
+        }
+      }
+
+      B.createBranch(SILLocation(Loc), finishBB);
+    }
+  }
+
   emitProfilerIncrement(dd->getTypecheckedBody());
   // Emit the destructor body.
+  if (deinitBodyBB)
+    B.emitBlock(deinitBodyBB);
   emitStmt(dd->getTypecheckedBody());
 
   Optional<SILValue> maybeReturnValue;
@@ -49,8 +94,6 @@ void SILGenFunction::emitDestroyingDestructor(DestructorDecl *dd) {
   if (!maybeReturnValue)
     return;
 
-  auto cleanupLoc = CleanupLocation(Loc);
-
   // If we have a superclass, invoke its destructor.
   SILValue resultSelfValue;
   SILType objectPtrTy = SILType::getNativeObjectType(F.getASTContext());
@@ -78,22 +121,6 @@ void SILGenFunction::emitDestroyingDestructor(DestructorDecl *dd) {
     resultSelfValue = selfValue;
   }
 
-  /// A distributed actor resigns its identity as it is deallocated.
-  /// This way the transport knows it must not deliver any more messages to it,
-  /// and can remove it from its (weak) lookup tables.
-  if (cd->isDistributedActor()) {
-    SILBasicBlock *continueBB = createBasicBlock();
-    
-    RegularLocation loc(dd);
-    if (dd->isImplicit())
-      loc.markAutoGenerated();
-    
-    // FIXME: what should the type of management be for this?
-    auto managedSelf = ManagedValue::forBorrowedRValue(selfValue);
-    emitConditionalResignIdentityCall(loc, cd, managedSelf, continueBB);
-    B.emitBlock(continueBB);
-  }
-
   ArgumentScope S(*this, Loc);
   ManagedValue borrowedValue =
       ManagedValue::forUnmanaged(resultSelfValue).borrow(*this, cleanupLoc);
@@ -103,8 +130,18 @@ void SILGenFunction::emitDestroyingDestructor(DestructorDecl *dd) {
         B.createUncheckedRefCast(cleanupLoc, borrowedValue, classTy);
   }
 
+  // A distributed actor must invoke `actorSystem.resignID` as it deinits.
+  if (cd->isDistributedActor()) {
+    // This must only be called by a *local* distributed actor (not a remote proxy).
+    // Since this call is emitted after the user-declared body of the deinit,
+    // just before returning; this is guaranteed to only be executed in the local
+    // actor case - because the body is never executed for a remote proxy either.
+    emitDistributedActorSystemResignIDCall(
+        cleanupLoc, cd, ManagedValue::forBorrowedRValue(selfValue));
+  }
+
   // Release our members.
-  emitClassMemberDestruction(borrowedValue, cd, cleanupLoc);
+  emitClassMemberDestruction(borrowedValue, cd, cleanupLoc, finishBB);
 
   S.pop();
 
@@ -199,7 +236,7 @@ void SILGenFunction::emitIVarDestroyer(SILDeclRef ivarDestroyer) {
         cleanupLoc, selfValue.forward(*this), OwnershipKind::Guaranteed);
       selfValue = emitManagedBorrowedRValueWithCleanup(guaranteedSelf);
     }
-    emitClassMemberDestruction(selfValue, cd, cleanupLoc);
+    emitClassMemberDestruction(selfValue, cd, cleanupLoc, /*finishBB=*/nullptr);
   }
 
   B.createReturn(loc, emitEmptyTuple(loc));
@@ -359,32 +396,10 @@ void SILGenFunction::emitRecursiveChainDestruction(ManagedValue selfValue,
 
 void SILGenFunction::emitClassMemberDestruction(ManagedValue selfValue,
                                                 ClassDecl *cd,
-                                                CleanupLocation cleanupLoc) {
+                                                CleanupLocation cleanupLoc,
+                                                SILBasicBlock *finishBB) {
   assert(selfValue.getOwnershipKind() == OwnershipKind::Guaranteed);
 
-  /// If this ClassDecl is a distributed actor, we must synthesise another code
-  /// path for deallocating a 'remote' actor. In that case, these basic blocks
-  /// are used to return to the "normal" (i.e. 'local' instance) destruction.
-  ///
-  /// For other cases, the basic blocks are not necessary and the destructor
-  /// can just emit all the normal destruction code right into the current block.
-  // If set, used as the basic block for the destroying of all members.
-  SILBasicBlock *normalMemberDestroyBB = nullptr;
-  // If set, used as the basic block after members have been destroyed,
-  // and we're ready to perform final cleanups before returning.
-  SILBasicBlock *finishBB = nullptr;
-
-  /// A distributed actor may be 'remote' in which case there is no need to
-  /// destroy "all" members, because they never had storage to begin with.
-  if (cd->isDistributedActor()) {
-    finishBB = createBasicBlock();
-    normalMemberDestroyBB = createBasicBlock();
-
-    emitDistributedActorClassMemberDestruction(cleanupLoc, selfValue, cd,
-                                               normalMemberDestroyBB,
-                                               finishBB);
-  }
-
   // Before we destroy all fields, we check if any of them are
   // recursively the same type as `self`, so we can iteratively
   // deinitialize them, to prevent deep recursion and potential
@@ -395,9 +410,6 @@ void SILGenFunction::emitClassMemberDestruction(ManagedValue selfValue,
 
   /// Destroy all members.
   {
-    if (normalMemberDestroyBB)
-      B.emitBlock(normalMemberDestroyBB);
-
     for (VarDecl *vd : cd->getStoredProperties()) {
       if (recursiveLinks.contains(vd))
         continue;

lib/SILGen/SILGenDistributed.cpp

@@ -86,28 +86,28 @@ static void initializeProperty(SILGenFunction &SGF, SILLocation loc,
 ///   <isLocalBB>
 /// }
 /// \endverbatim
-static void emitDistributedIfRemoteBranch(SILGenFunction &SGF, SILLocation Loc,
-                                          ManagedValue selfValue, Type selfTy,
-                                          SILBasicBlock *isRemoteBB,
-                                          SILBasicBlock *isLocalBB) {
-  ASTContext &ctx = SGF.getASTContext();
-  auto &B = SGF.B;
+void SILGenFunction::emitDistributedIfRemoteBranch(SILLocation Loc,
+                                                   ManagedValue selfValue,
+                                                   Type selfTy,
+                                                   SILBasicBlock *isRemoteBB,
+                                                   SILBasicBlock *isLocalBB) {
+  ASTContext &ctx = getASTContext();
 
   FuncDecl *isRemoteFn = ctx.getIsRemoteDistributedActor();
   assert(isRemoteFn && "Could not find 'is remote' function, is the "
                        "'Distributed' module available?");
 
   ManagedValue selfAnyObject = B.createInitExistentialRef(
       Loc,
-      /*existentialType=*/SGF.getLoweredType(ctx.getAnyObjectType()),
+      /*existentialType=*/getLoweredType(ctx.getAnyObjectType()),
       /*formalConcreteType=*/selfValue.getType().getASTType(),
       selfValue, {});
-  auto result = SGF.emitApplyOfLibraryIntrinsic(
+  auto result = emitApplyOfLibraryIntrinsic(
       Loc, isRemoteFn, SubstitutionMap(), {selfAnyObject}, SGFContext());
 
-  SILValue isRemoteResult = std::move(result).forwardAsSingleValue(SGF, Loc);
+  SILValue isRemoteResult = std::move(result).forwardAsSingleValue(*this, Loc);
   SILValue isRemoteResultUnwrapped =
-      SGF.emitUnwrapIntegerResult(Loc, isRemoteResult);
+      emitUnwrapIntegerResult(Loc, isRemoteResult);
 
   B.createCondBranch(Loc, isRemoteResultUnwrapped, isRemoteBB, isLocalBB);
 }
@@ -511,9 +511,12 @@ void
 SILGenFunction::emitConditionalResignIdentityCall(SILLocation loc,
                                                   ClassDecl *actorDecl,
                                                   ManagedValue actorSelf,
-                                                  SILBasicBlock *continueBB) {
+                                                  SILBasicBlock *continueBB,
+                                                  SILBasicBlock *finishBB) {
   assert(actorDecl->isDistributedActor() &&
-  "only distributed actors have actorSystem lifecycle hooks in deinit");
+         "only distributed actors have actorSystem lifecycle hooks in deinit");
+  assert(continueBB && finishBB &&
+         "need valid continue and finish basic blocks");
 
   auto selfTy = F.mapTypeIntoContext(actorDecl->getDeclaredInterfaceType());
 
@@ -527,15 +530,15 @@ SILGenFunction::emitConditionalResignIdentityCall(SILLocation loc,
   // } else {
   //   ...
   // }
-  emitDistributedIfRemoteBranch(*this, loc,
+  emitDistributedIfRemoteBranch(loc,
                                 actorSelf, selfTy,
                                 /*if remote*/isRemoteBB,
                                 /*if local*/isLocalBB);
 
-  // if remote, do nothing.
+  // if remote, return early; the user defined deinit should not run.
   {
     B.emitBlock(isRemoteBB);
-    B.createBranch(loc, continueBB);
+    B.createBranch(loc, finishBB);
   }
 
   // if local, resign identity.
@@ -554,20 +557,21 @@ SILGenFunction::emitConditionalResignIdentityCall(SILLocation loc,
 
 void SILGenFunction::emitDistributedActorClassMemberDestruction(
     SILLocation cleanupLoc, ManagedValue selfValue, ClassDecl *cd,
-    SILBasicBlock *normalMemberDestroyBB, SILBasicBlock *finishBB) {
+    SILBasicBlock *normalMemberDestroyBB,
+    SILBasicBlock *remoteMemberDestroyBB,
+    SILBasicBlock *finishBB) {
   auto selfTy = cd->getDeclaredInterfaceType();
 
   Scope scope(Cleanups, CleanupLocation(cleanupLoc));
 
   auto isLocalBB = createBasicBlock("isLocalBB");
-  auto remoteMemberDestroyBB = createBasicBlock("remoteMemberDestroyBB");
 
   // if __isRemoteActor(self) {
   //   ...
   // } else {
   //   ...
   // }
-  emitDistributedIfRemoteBranch(*this, cleanupLoc,
+  emitDistributedIfRemoteBranch(cleanupLoc,
                                 selfValue, selfTy,
                                 /*if remote*/remoteMemberDestroyBB,
                                 /*if local*/isLocalBB);

lib/SILGen/SILGenFunction.h

@@ -685,8 +685,12 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   ///
   /// \param selfValue The 'self' value.
   /// \param cd The class declaration whose members are being destroyed.
+  /// \param finishBB If set, used as the basic block after members have been
+  ///                 destroyed, and we're ready to perform final cleanups
+  ///                 before returning.
   void emitClassMemberDestruction(ManagedValue selfValue, ClassDecl *cd,
-                                  CleanupLocation cleanupLoc);
+                                  CleanupLocation cleanupLoc,
+                                  SILBasicBlock* finishBB);
 
   /// Generates code to destroy linearly recursive data structures, without
   /// building up the call stack.
@@ -2096,8 +2100,12 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
 
   /// Given a function representing a distributed actor factory, emits the
   /// corresponding SIL function for it.
-  void emitDistributedActorFactory(
-      FuncDecl *fd); // TODO(distributed): this is the "resolve"
+  void emitDistributedActorFactory(FuncDecl *fd); // TODO(distributed): this is the "resolve"
+
+  void emitDistributedIfRemoteBranch(SILLocation Loc,
+                                     ManagedValue selfValue, Type selfTy,
+                                     SILBasicBlock *isRemoteBB,
+                                     SILBasicBlock *isLocalBB);
 
   /// Notify transport that actor has initialized successfully,
   /// and is ready to receive messages.
@@ -2127,11 +2135,14 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   void emitConditionalResignIdentityCall(SILLocation loc,
                                          ClassDecl *actorDecl,
                                          ManagedValue actorSelf,
-                                         SILBasicBlock *continueBB);
+                                         SILBasicBlock *continueBB,
+                                         SILBasicBlock *finishBB);
 
   void emitDistributedActorClassMemberDestruction(
       SILLocation cleanupLoc, ManagedValue selfValue, ClassDecl *cd,
-      SILBasicBlock *normalMemberDestroyBB, SILBasicBlock *finishBB);
+      SILBasicBlock *normalMemberDestroyBB,
+      SILBasicBlock *remoteMemberDestroyBB,
+      SILBasicBlock *finishBB);
 
   //===--------------------------------------------------------------------===//
   // Declarations

test/Distributed/Runtime/distributed_actor_deinit.swift

@@ -34,8 +34,7 @@ distributed actor DA_userDefined2 {
   }
 
   deinit {
-    print("Deinitializing \(self.id)")
-    return
+    print("Deinitializing \(self.id) remote:\(__isRemoteActor(self))")
   }
 }
 
@@ -48,7 +47,7 @@ distributed actor DA_state {
   }
 
   deinit {
-    print("Deinitializing \(self.id)")
+    print("Deinitializing \(self.id) remote:\(__isRemoteActor(self))")
     return
   }
 }
@@ -204,7 +203,7 @@ func test() {
   }()
   // CHECK: assign type:DA_userDefined2, address:[[ADDRESS:.*]]
   // CHECK: ready actor:main.DA_userDefined2, address:ActorAddress(address: "[[ADDR3:addr-[0-9]]]")
-  // CHECK: Deinitializing ActorAddress(address: "[[ADDR3]]")
+  // CHECK: Deinitializing ActorAddress(address: "[[ADDR3]]") remote:false
   // CHECK-NEXT: resign address:ActorAddress(address: "[[ADDR3]]")
 
   // resign must happen as the _last thing_ after user-deinit completed
@@ -213,7 +212,7 @@ func test() {
   }()
   // CHECK: assign type:DA_state, address:[[ADDRESS:.*]]
   // CHECK: ready actor:main.DA_state, address:ActorAddress(address: "[[ADDR4:addr-[0-9]]]")
-  // CHECK: Deinitializing ActorAddress(address: "[[ADDR4]]")
+  // CHECK: Deinitializing ActorAddress(address: "[[ADDR4]]") remote:false
   // CHECK-NEXT: resign address:ActorAddress(address: "[[ADDR4]]")
 
   // a remote actor should not resign it's address, it was never "assigned" it
@@ -222,7 +221,11 @@ func test() {
     try! DA_userDefined2.resolve(id: address, using: system)
   }()
   // CHECK-NEXT: resolve type:DA_userDefined2, address:ActorAddress(address: "[[ADDR5:remote-1]]")
-  // CHECK-NEXT: Deinitializing
+  // MUST NOT run deinit body for a remote distributed actor
+  // CHECK-NOT: Deinitializing ActorAddress(address: "remote-1") remote:true
+
+  print("DONE")
+  // CHECK-NEXT: DONE
 }
 
 @main struct Main {

test/Distributed/Runtime/distributed_actor_remote_fieldsDontCrashDeinit.swift

@@ -48,9 +48,11 @@ func test_remote() async {
       try! SomeSpecificDistributedActor.resolve(id: address, using: system)
   // Check the id and system are the right values, and not trash memory
   print("remote.id = \(remote!.id)") // CHECK: remote.id = ActorAddress(address: "sact://127.0.0.1/example#1234")
-  print("remote.system = \(remote!.actorSystem)")
+  print("remote.system = \(remote!.actorSystem)") // CHECK: remote.system = FakeActorSystem()
 
-  remote = nil // CHECK: deinit ActorAddress(address: "sact://127.0.0.1/example#1234")
+  remote = nil
+  // CHECK-NOT: deinit ActorAddress(address: "sact://127.0.0.1/example#1234")
+  // CHECK-NEXT: done
   print("done")
 }
 

test/Distributed/Runtime/distributed_actor_remote_retains_system.swift

@@ -147,8 +147,6 @@ func test_remote() async {
   print("remote.id = \(remote.id)") // CHECK: remote.id = ActorAddress(address: "sact://127.0.0.1/example#1234")
   print("remote.system = \(remote.actorSystem)") // CHECK: remote.system = main.FakeActorSystem
 
-  // only once we exit the function and the remote is released, the system has no more references
-  // CHECK-DAG: deinit ActorAddress(address: "sact://127.0.0.1/example#1234")
   // system must deinit after the last actor using it does deinit
   // CHECK-DAG: deinit main.FakeActorSystem
 }