Skip to content

Commit 60c015d

Browse files
ktosoktoso
committed
[Distributed] Prevent remote distributed actor from running deinit body
1 parent 98ba11f commit 60c015d

8 files changed

+193
-153
lines changed

‎lib/SIL/IR/SILPrinter.cpp

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -894,6 +894,7 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
894894
llvm::SmallVector<SILValue, 8> values;
895895
llvm::copy(inst->getResults(), std::back_inserter(values));
896896
printUserList(values, inst);
897+
printBranchTargets(inst);
897898
}
898899

899900
void printUserList(ArrayRef<SILValue> values, SILNodePointer node) {
@@ -937,6 +938,21 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
937938
[&] { *this << ", "; });
938939
}
939940

941+
void printBranchTargets(const SILInstruction *inst) {
942+
if (auto condBr = dyn_cast<CondBranchInst>(inst)) {
943+
if (condBr->getTrueBB()->getDebugName().hasValue()) {
944+
*this << ", true->" << condBr->getTrueBB()->getDebugName().getValue();
945+
}
946+
if (condBr->getFalseBB()->getDebugName().hasValue()) {
947+
*this << ", false->" << condBr->getFalseBB()->getDebugName().getValue();
948+
}
949+
} else if (auto br = dyn_cast<BranchInst>(inst)) {
950+
if (br->getDestBB()->getDebugName().hasValue()) {
951+
*this << ", dest->" << br->getDestBB()->getDebugName().getValue();
952+
}
953+
}
954+
}
955+
940956
void printConformances(ArrayRef<ProtocolConformanceRef> conformances) {
941957
// FIXME: conformances should always be printed and parsed!
942958
if (!Ctx.printVerbose()) {

‎lib/SILGen/SILGenDestructor.cpp

Lines changed: 59 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,60 @@ void SILGenFunction::emitDestroyingDestructor(DestructorDecl *dd) {
3131
Loc.markAutoGenerated();
3232

3333
auto cd = cast<ClassDecl>(dd->getDeclContext());
34+
auto &C = cd->getASTContext();
3435
SILValue selfValue = emitSelfDecl(dd->getImplicitSelfDecl());
3536

3637
// Create a basic block to jump to for the implicit destruction behavior
3738
// of releasing the elements and calling the superclass destructor.
3839
// We won't actually emit the block until we finish with the destructor body.
3940
prepareEpilog(None, false, CleanupLocation(Loc));
4041

42+
auto cleanupLoc = CleanupLocation(Loc);
43+
44+
SILBasicBlock *deinitBodyBB = nullptr;
45+
SILBasicBlock *finishBB = nullptr;
46+
if (cd->isDistributedActor()) {
47+
auto remoteBB = createBasicBlock("remoteActorDeinitBB");
48+
finishBB = createBasicBlock("finishDeinitBB");
49+
deinitBodyBB = createBasicBlock("deinitBodyBB");
50+
51+
// FIXME: what should the type of management be for this?
52+
auto managedSelf = ManagedValue::forBorrowedRValue(selfValue);
53+
54+
auto selfTy = F.mapTypeIntoContext(cd->getDeclaredInterfaceType());
55+
emitDistributedIfRemoteBranch(
56+
SILLocation(Loc),
57+
managedSelf, selfTy,
58+
/*if remote=*/remoteBB,
59+
/*if local=*/deinitBodyBB);
60+
61+
{
62+
B.emitBlock(remoteBB);
63+
64+
// Note that we do NOT execute user-declared the deinit body.
65+
// They would be free to access state which does not exist in a remote DA
66+
67+
// we are a remote instance,
68+
// the only properties we can destroy are the id and system properties.
69+
for (VarDecl *vd : cd->getStoredProperties()) {
70+
if (getActorIsolation(vd) == ActorIsolation::ActorInstance)
71+
continue;
72+
73+
// Just to double-check, we only want to destroy `id` and `actorSystem`
74+
if (vd->getBaseIdentifier() == C.Id_id ||
75+
vd->getBaseIdentifier() == C.Id_actorSystem) {
76+
destroyClassMember(cleanupLoc, managedSelf, vd);
77+
}
78+
}
79+
80+
B.createBranch(SILLocation(Loc), finishBB);
81+
}
82+
}
83+
4184
emitProfilerIncrement(dd->getTypecheckedBody());
4285
// Emit the destructor body.
86+
if (deinitBodyBB)
87+
B.emitBlock(deinitBodyBB);
4388
emitStmt(dd->getTypecheckedBody());
4489

4590
Optional<SILValue> maybeReturnValue;
@@ -49,8 +94,6 @@ void SILGenFunction::emitDestroyingDestructor(DestructorDecl *dd) {
4994
if (!maybeReturnValue)
5095
return;
5196

52-
auto cleanupLoc = CleanupLocation(Loc);
53-
5497
// If we have a superclass, invoke its destructor.
5598
SILValue resultSelfValue;
5699
SILType objectPtrTy = SILType::getNativeObjectType(F.getASTContext());
@@ -78,22 +121,6 @@ void SILGenFunction::emitDestroyingDestructor(DestructorDecl *dd) {
78121
resultSelfValue = selfValue;
79122
}
80123

81-
/// A distributed actor resigns its identity as it is deallocated.
82-
/// This way the transport knows it must not deliver any more messages to it,
83-
/// and can remove it from its (weak) lookup tables.
84-
if (cd->isDistributedActor()) {
85-
SILBasicBlock *continueBB = createBasicBlock();
86-
87-
RegularLocation loc(dd);
88-
if (dd->isImplicit())
89-
loc.markAutoGenerated();
90-
91-
// FIXME: what should the type of management be for this?
92-
auto managedSelf = ManagedValue::forBorrowedRValue(selfValue);
93-
emitConditionalResignIdentityCall(loc, cd, managedSelf, continueBB);
94-
B.emitBlock(continueBB);
95-
}
96-
97124
ArgumentScope S(*this, Loc);
98125
ManagedValue borrowedValue =
99126
ManagedValue::forUnmanaged(resultSelfValue).borrow(*this, cleanupLoc);
@@ -103,8 +130,18 @@ void SILGenFunction::emitDestroyingDestructor(DestructorDecl *dd) {
103130
B.createUncheckedRefCast(cleanupLoc, borrowedValue, classTy);
104131
}
105132

133+
// A distributed actor must invoke `actorSystem.resignID` as it deinits.
134+
if (cd->isDistributedActor()) {
135+
// This must only be called by a *local* distributed actor (not a remote proxy).
136+
// Since this call is emitted after the user-declared body of the deinit,
137+
// just before returning; this is guaranteed to only be executed in the local
138+
// actor case - because the body is never executed for a remote proxy either.
139+
emitDistributedActorSystemResignIDCall(
140+
cleanupLoc, cd, ManagedValue::forBorrowedRValue(selfValue));
141+
}
142+
106143
// Release our members.
107-
emitClassMemberDestruction(borrowedValue, cd, cleanupLoc);
144+
emitClassMemberDestruction(borrowedValue, cd, cleanupLoc, finishBB);
108145

109146
S.pop();
110147

@@ -199,7 +236,7 @@ void SILGenFunction::emitIVarDestroyer(SILDeclRef ivarDestroyer) {
199236
cleanupLoc, selfValue.forward(*this), OwnershipKind::Guaranteed);
200237
selfValue = emitManagedBorrowedRValueWithCleanup(guaranteedSelf);
201238
}
202-
emitClassMemberDestruction(selfValue, cd, cleanupLoc);
239+
emitClassMemberDestruction(selfValue, cd, cleanupLoc, /*finishBB=*/nullptr);
203240
}
204241

205242
B.createReturn(loc, emitEmptyTuple(loc));
@@ -359,32 +396,10 @@ void SILGenFunction::emitRecursiveChainDestruction(ManagedValue selfValue,
359396

360397
void SILGenFunction::emitClassMemberDestruction(ManagedValue selfValue,
361398
ClassDecl *cd,
362-
CleanupLocation cleanupLoc) {
399+
CleanupLocation cleanupLoc,
400+
SILBasicBlock *finishBB) {
363401
assert(selfValue.getOwnershipKind() == OwnershipKind::Guaranteed);
364402

365-
/// If this ClassDecl is a distributed actor, we must synthesise another code
366-
/// path for deallocating a 'remote' actor. In that case, these basic blocks
367-
/// are used to return to the "normal" (i.e. 'local' instance) destruction.
368-
///
369-
/// For other cases, the basic blocks are not necessary and the destructor
370-
/// can just emit all the normal destruction code right into the current block.
371-
// If set, used as the basic block for the destroying of all members.
372-
SILBasicBlock *normalMemberDestroyBB = nullptr;
373-
// If set, used as the basic block after members have been destroyed,
374-
// and we're ready to perform final cleanups before returning.
375-
SILBasicBlock *finishBB = nullptr;
376-
377-
/// A distributed actor may be 'remote' in which case there is no need to
378-
/// destroy "all" members, because they never had storage to begin with.
379-
if (cd->isDistributedActor()) {
380-
finishBB = createBasicBlock();
381-
normalMemberDestroyBB = createBasicBlock();
382-
383-
emitDistributedActorClassMemberDestruction(cleanupLoc, selfValue, cd,
384-
normalMemberDestroyBB,
385-
finishBB);
386-
}
387-
388403
// Before we destroy all fields, we check if any of them are
389404
// recursively the same type as `self`, so we can iteratively
390405
// deinitialize them, to prevent deep recursion and potential
@@ -395,9 +410,6 @@ void SILGenFunction::emitClassMemberDestruction(ManagedValue selfValue,
395410

396411
/// Destroy all members.
397412
{
398-
if (normalMemberDestroyBB)
399-
B.emitBlock(normalMemberDestroyBB);
400-
401413
for (VarDecl *vd : cd->getStoredProperties()) {
402414
if (recursiveLinks.contains(vd))
403415
continue;

‎lib/SILGen/SILGenDistributed.cpp

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -86,28 +86,28 @@ static void initializeProperty(SILGenFunction &SGF, SILLocation loc,
8686
/// <isLocalBB>
8787
/// }
8888
/// \endverbatim
89-
static void emitDistributedIfRemoteBranch(SILGenFunction &SGF, SILLocation Loc,
90-
ManagedValue selfValue, Type selfTy,
91-
SILBasicBlock *isRemoteBB,
92-
SILBasicBlock *isLocalBB) {
93-
ASTContext &ctx = SGF.getASTContext();
94-
auto &B = SGF.B;
89+
void SILGenFunction::emitDistributedIfRemoteBranch(SILLocation Loc,
90+
ManagedValue selfValue,
91+
Type selfTy,
92+
SILBasicBlock *isRemoteBB,
93+
SILBasicBlock *isLocalBB) {
94+
ASTContext &ctx = getASTContext();
9595

9696
FuncDecl *isRemoteFn = ctx.getIsRemoteDistributedActor();
9797
assert(isRemoteFn && "Could not find 'is remote' function, is the "
9898
"'Distributed' module available?");
9999

100100
ManagedValue selfAnyObject = B.createInitExistentialRef(
101101
Loc,
102-
/*existentialType=*/SGF.getLoweredType(ctx.getAnyObjectType()),
102+
/*existentialType=*/getLoweredType(ctx.getAnyObjectType()),
103103
/*formalConcreteType=*/selfValue.getType().getASTType(),
104104
selfValue, {});
105-
auto result = SGF.emitApplyOfLibraryIntrinsic(
105+
auto result = emitApplyOfLibraryIntrinsic(
106106
Loc, isRemoteFn, SubstitutionMap(), {selfAnyObject}, SGFContext());
107107

108-
SILValue isRemoteResult = std::move(result).forwardAsSingleValue(SGF, Loc);
108+
SILValue isRemoteResult = std::move(result).forwardAsSingleValue(*this, Loc);
109109
SILValue isRemoteResultUnwrapped =
110-
SGF.emitUnwrapIntegerResult(Loc, isRemoteResult);
110+
emitUnwrapIntegerResult(Loc, isRemoteResult);
111111

112112
B.createCondBranch(Loc, isRemoteResultUnwrapped, isRemoteBB, isLocalBB);
113113
}
@@ -511,9 +511,12 @@ void
511511
SILGenFunction::emitConditionalResignIdentityCall(SILLocation loc,
512512
ClassDecl *actorDecl,
513513
ManagedValue actorSelf,
514-
SILBasicBlock *continueBB) {
514+
SILBasicBlock *continueBB,
515+
SILBasicBlock *finishBB) {
515516
assert(actorDecl->isDistributedActor() &&
516-
"only distributed actors have actorSystem lifecycle hooks in deinit");
517+
"only distributed actors have actorSystem lifecycle hooks in deinit");
518+
assert(continueBB && finishBB &&
519+
"need valid continue and finish basic blocks");
517520

518521
auto selfTy = F.mapTypeIntoContext(actorDecl->getDeclaredInterfaceType());
519522

@@ -527,15 +530,15 @@ SILGenFunction::emitConditionalResignIdentityCall(SILLocation loc,
527530
// } else {
528531
// ...
529532
// }
530-
emitDistributedIfRemoteBranch(*this, loc,
533+
emitDistributedIfRemoteBranch(loc,
531534
actorSelf, selfTy,
532535
/*if remote*/isRemoteBB,
533536
/*if local*/isLocalBB);
534537

535-
// if remote, do nothing.
538+
// if remote, return early; the user defined deinit should not run.
536539
{
537540
B.emitBlock(isRemoteBB);
538-
B.createBranch(loc, continueBB);
541+
B.createBranch(loc, finishBB);
539542
}
540543

541544
// if local, resign identity.
@@ -554,20 +557,22 @@ SILGenFunction::emitConditionalResignIdentityCall(SILLocation loc,
554557

555558
void SILGenFunction::emitDistributedActorClassMemberDestruction(
556559
SILLocation cleanupLoc, ManagedValue selfValue, ClassDecl *cd,
557-
SILBasicBlock *normalMemberDestroyBB, SILBasicBlock *finishBB) {
560+
SILBasicBlock *normalMemberDestroyBB,
561+
SILBasicBlock *remoteMemberDestroyBB,
562+
SILBasicBlock *finishBB) {
558563
auto selfTy = cd->getDeclaredInterfaceType();
559564

560565
Scope scope(Cleanups, CleanupLocation(cleanupLoc));
561566

562567
auto isLocalBB = createBasicBlock("isLocalBB");
563-
auto remoteMemberDestroyBB = createBasicBlock("remoteMemberDestroyBB");
568+
// auto remoteMemberDestroyBB = createBasicBlock("remoteMemberDestroyBB");
564569

565570
// if __isRemoteActor(self) {
566571
// ...
567572
// } else {
568573
// ...
569574
// }
570-
emitDistributedIfRemoteBranch(*this, cleanupLoc,
575+
emitDistributedIfRemoteBranch(cleanupLoc,
571576
selfValue, selfTy,
572577
/*if remote*/remoteMemberDestroyBB,
573578
/*if local*/isLocalBB);

‎lib/SILGen/SILGenFunction.h

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -685,8 +685,12 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
685685
///
686686
/// \param selfValue The 'self' value.
687687
/// \param cd The class declaration whose members are being destroyed.
688+
/// \param finishBB If set, used as the basic block after members have been
689+
/// destroyed, and we're ready to perform final cleanups
690+
/// before returning.
688691
void emitClassMemberDestruction(ManagedValue selfValue, ClassDecl *cd,
689-
CleanupLocation cleanupLoc);
692+
CleanupLocation cleanupLoc,
693+
SILBasicBlock* finishBB);
690694

691695
/// Generates code to destroy linearly recursive data structures, without
692696
/// building up the call stack.
@@ -2096,8 +2100,12 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
20962100

20972101
/// Given a function representing a distributed actor factory, emits the
20982102
/// corresponding SIL function for it.
2099-
void emitDistributedActorFactory(
2100-
FuncDecl *fd); // TODO(distributed): this is the "resolve"
2103+
void emitDistributedActorFactory(FuncDecl *fd); // TODO(distributed): this is the "resolve"
2104+
2105+
void emitDistributedIfRemoteBranch(SILLocation Loc,
2106+
ManagedValue selfValue, Type selfTy,
2107+
SILBasicBlock *isRemoteBB,
2108+
SILBasicBlock *isLocalBB);
21012109

21022110
/// Notify transport that actor has initialized successfully,
21032111
/// and is ready to receive messages.
@@ -2127,11 +2135,14 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
21272135
void emitConditionalResignIdentityCall(SILLocation loc,
21282136
ClassDecl *actorDecl,
21292137
ManagedValue actorSelf,
2130-
SILBasicBlock *continueBB);
2138+
SILBasicBlock *continueBB,
2139+
SILBasicBlock *finishBB);
21312140

21322141
void emitDistributedActorClassMemberDestruction(
21332142
SILLocation cleanupLoc, ManagedValue selfValue, ClassDecl *cd,
2134-
SILBasicBlock *normalMemberDestroyBB, SILBasicBlock *finishBB);
2143+
SILBasicBlock *normalMemberDestroyBB,
2144+
SILBasicBlock *remoteMemberDestroyBB,
2145+
SILBasicBlock *finishBB);
21352146

21362147
//===--------------------------------------------------------------------===//
21372148
// Declarations

‎test/Distributed/Runtime/distributed_actor_deinit.swift

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,7 @@ distributed actor DA_userDefined2 {
3434
}
3535

3636
deinit {
37-
print("Deinitializing \(self.id)")
38-
return
37+
print("Deinitializing \(self.id) remote:\(__isRemoteActor(self))")
3938
}
4039
}
4140

@@ -48,7 +47,7 @@ distributed actor DA_state {
4847
}
4948

5049
deinit {
51-
print("Deinitializing \(self.id)")
50+
print("Deinitializing \(self.id) remote:\(__isRemoteActor(self))")
5251
return
5352
}
5453
}
@@ -204,7 +203,7 @@ func test() {
204203
}()
205204
// CHECK: assign type:DA_userDefined2, address:[[ADDRESS:.*]]
206205
// CHECK: ready actor:main.DA_userDefined2, address:ActorAddress(address: "[[ADDR3:addr-[0-9]]]")
207-
// CHECK: Deinitializing ActorAddress(address: "[[ADDR3]]")
206+
// CHECK: Deinitializing ActorAddress(address: "[[ADDR3]]") remote:false
208207
// CHECK-NEXT: resign address:ActorAddress(address: "[[ADDR3]]")
209208

210209
// resign must happen as the _last thing_ after user-deinit completed
@@ -213,7 +212,7 @@ func test() {
213212
}()
214213
// CHECK: assign type:DA_state, address:[[ADDRESS:.*]]
215214
// CHECK: ready actor:main.DA_state, address:ActorAddress(address: "[[ADDR4:addr-[0-9]]]")
216-
// CHECK: Deinitializing ActorAddress(address: "[[ADDR4]]")
215+
// CHECK: Deinitializing ActorAddress(address: "[[ADDR4]]") remote:false
217216
// CHECK-NEXT: resign address:ActorAddress(address: "[[ADDR4]]")
218217

219218
// a remote actor should not resign it's address, it was never "assigned" it
@@ -222,7 +221,11 @@ func test() {
222221
try! DA_userDefined2.resolve(id: address, using: system)
223222
}()
224223
// CHECK-NEXT: resolve type:DA_userDefined2, address:ActorAddress(address: "[[ADDR5:remote-1]]")
225-
// CHECK-NEXT: Deinitializing
224+
// MUST NOT run deinit body for a remote distributed actor
225+
// CHECK-NOT: Deinitializing ActorAddress(address: "remote-1") remote:true
226+
227+
print("DONE")
228+
// CHECK-NEXT: DONE
226229
}
227230

228231
@main struct Main {

0 commit comments

Comments
 (0)