Skip to content

Commit 06aa334

Browse files
drexindrexin
committed
Address review feedback
1 parent 542c395 commit 06aa334

File tree

5 files changed

+102
-56
lines changed

5 files changed

+102
-56
lines changed

‎lib/SILGen/SILGenDestructor.cpp

Lines changed: 85 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "RValue.h"
1515
#include "SILGenFunction.h"
1616
#include "SILGenFunctionBuilder.h"
17+
#include "SwitchEnumBuilder.h"
1718
#include "swift/AST/GenericSignature.h"
1819
#include "swift/AST/SubstitutionMap.h"
1920
#include "swift/SIL/TypeLowering.h"
@@ -220,21 +221,34 @@ void SILGenFunction::destroyClassMember(SILLocation cleanupLoc,
220221
}
221222
}
222223

224+
// Finds stored properties that have the same type as `cd` and thus form
225+
// a recursive structure.
226+
//
227+
// Example:
228+
//
229+
// class Node<T> {
230+
// let element: T
231+
// let next: Node<T>?
232+
// }
233+
//
234+
// In the above example `next` is a recursive link and would be recognized
235+
// by this function and added to the result set.
223236
void findRecursiveLinks(ClassDecl *cd, llvm::SmallSetVector<VarDecl*, 4> &result) {
224-
auto SelfTy = cd->getDeclaredInterfaceType();
237+
auto selfTy = cd->getDeclaredInterfaceType();
225238

226239
// Collect all stored properties that would form a recursive structure,
227240
// so we can remove the recursion and prevent the call stack from
228241
// overflowing.
229242
for (VarDecl *vd : cd->getStoredProperties()) {
230243
auto Ty = vd->getInterfaceType()->getOptionalObjectType();
231-
if (Ty && Ty->getCanonicalType() == SelfTy->getCanonicalType()) {
244+
if (Ty && Ty->getCanonicalType() == selfTy->getCanonicalType()) {
232245
result.insert(vd);
233246
}
234247
}
235248

236-
// NOTE: Right now we only optimize linear recursion, so if there is more than
237-
// one link, clear out the set and don't perform any recursion optimization.
249+
// NOTE: Right now we only optimize linear recursion, so if there is more
250+
// than one stored property of the same type, clear out the set and don't
251+
// perform any recursion optimization.
238252
if (result.size() > 1) {
239253
result.clear();
240254
}
@@ -244,9 +258,9 @@ void SILGenFunction::emitRecursiveChainDestruction(ManagedValue selfValue,
244258
ClassDecl *cd,
245259
VarDecl *recursiveLink,
246260
CleanupLocation cleanupLoc) {
247-
auto SelfTy = F.mapTypeIntoContext(cd->getDeclaredInterfaceType());
261+
auto selfTy = F.mapTypeIntoContext(cd->getDeclaredInterfaceType());
248262

249-
auto SelfTyLowered = getTypeLowering(SelfTy).getLoweredType();
263+
auto selfTyLowered = getTypeLowering(selfTy).getLoweredType();
250264

251265
SILBasicBlock *cleanBB = createBasicBlock();
252266
SILBasicBlock *noneBB = createBasicBlock();
@@ -262,7 +276,7 @@ void SILGenFunction::emitRecursiveChainDestruction(ManagedValue selfValue,
262276
SILValue varAddr =
263277
B.createRefElementAddr(cleanupLoc, selfValue.getValue(), recursiveLink,
264278
Ty.getAddressType());
265-
auto iterAddr = B.createAllocStack(cleanupLoc, Ty);
279+
auto *iterAddr = B.createAllocStack(cleanupLoc, Ty);
266280
SILValue addr = B.createBeginAccess(
267281
cleanupLoc, varAddr, SILAccessKind::Modify, SILAccessEnforcement::Static,
268282
true /*noNestedConflict*/, false /*fromBuiltin*/);
@@ -274,57 +288,77 @@ void SILGenFunction::emitRecursiveChainDestruction(ManagedValue selfValue,
274288
B.createBranch(cleanupLoc, loopBB);
275289

276290
// while iter != nil {
277-
B.emitBlock(loopBB);
278-
B.createSwitchEnumAddr(
279-
cleanupLoc, iterAddr, nullptr,
280-
{{getASTContext().getOptionalSomeDecl(), someBB},
281-
{std::make_pair(getASTContext().getOptionalNoneDecl(), noneBB)}});
291+
{
292+
B.emitBlock(loopBB);
293+
auto iterBorrow =
294+
ManagedValue::forUnmanaged(iterAddr).borrow(*this, cleanupLoc);
295+
SwitchEnumBuilder switchBuilder(B, cleanupLoc, iterBorrow);
296+
switchBuilder.addOptionalSomeCase(someBB);
297+
switchBuilder.addOptionalNoneCase(noneBB);
298+
std::move(switchBuilder).emit();
299+
}
282300

283301
// if isKnownUniquelyReferenced(&iter) {
284-
B.emitBlock(someBB);
285-
auto isUnique = B.createIsUnique(cleanupLoc, iterAddr);
286-
B.createCondBranch(cleanupLoc, isUnique, uniqueBB, notUniqueBB);
302+
{
303+
B.emitBlock(someBB);
304+
auto isUnique = B.createIsUnique(cleanupLoc, iterAddr);
305+
B.createCondBranch(cleanupLoc, isUnique, uniqueBB, notUniqueBB);
306+
}
287307

288308
// we have a uniquely referenced link, so we need to deinit
289-
B.emitBlock(uniqueBB);
290-
291-
// NOTE: We increment the ref count of the tail instead of unlinking it,
292-
// because custom deinit implementations of subclasses may access
293-
// it and it would be semantically wrong to unset it before that.
294-
// Making the tail non-uniquely referenced prevents the recursion.
295-
296-
// let tail = iter.unsafelyUnwrapped.next
297-
// iter = tail
298-
SILValue iterBorrow = B.createLoadBorrow(cleanupLoc, iterAddr);
299-
auto *link = B.createUncheckedEnumData(cleanupLoc, iterBorrow,
300-
getASTContext().getOptionalSomeDecl(),
301-
SelfTyLowered);
302-
303-
varAddr = B.createRefElementAddr(cleanupLoc, link, recursiveLink,
304-
Ty.getAddressType());
305-
306-
addr = B.createBeginAccess(
307-
cleanupLoc, varAddr, SILAccessKind::Read, SILAccessEnforcement::Static,
308-
true /* noNestedConflict */, false /*fromBuiltin*/);
309-
iter = B.createLoad(cleanupLoc, addr, LoadOwnershipQualifier::Copy);
310-
B.createEndAccess(cleanupLoc, addr, false /*is aborting*/);
311-
B.createEndBorrow(cleanupLoc, iterBorrow);
309+
{
310+
B.emitBlock(uniqueBB);
312311

313-
B.createStore(cleanupLoc, iter, iterAddr, StoreOwnershipQualifier::Assign);
312+
// NOTE: We increment the ref count of the tail instead of unlinking it,
313+
// because custom deinit implementations of subclasses may access
314+
// it and it would be semantically wrong to unset it before that.
315+
// Making the tail non-uniquely referenced prevents the recursion.
314316

315-
B.createBranch(cleanupLoc, loopBB);
317+
// let tail = iter.unsafelyUnwrapped.next
318+
// iter = tail
319+
SILValue iterBorrow = B.createLoadBorrow(cleanupLoc, iterAddr);
320+
auto *link = B.createUncheckedEnumData(
321+
cleanupLoc, iterBorrow, getASTContext().getOptionalSomeDecl(),
322+
selfTyLowered);
323+
324+
varAddr = B.createRefElementAddr(cleanupLoc, link, recursiveLink,
325+
Ty.getAddressType());
326+
327+
addr = B.createBeginAccess(
328+
cleanupLoc, varAddr, SILAccessKind::Read, SILAccessEnforcement::Static,
329+
true /* noNestedConflict */, false /*fromBuiltin*/);
330+
331+
// The deinit of `iter` will decrement the ref count of the field
332+
// containing the next element and potentially leading to its
333+
// deinitialization, causing the recursion. The prevent that,
334+
// we `load [copy]` here to ensure the object stays alive until
335+
// we explicitly release it in the next step of the iteration.
336+
iter = B.createLoad(cleanupLoc, addr, LoadOwnershipQualifier::Copy);
337+
B.createEndAccess(cleanupLoc, addr, false /*is aborting*/);
338+
B.createEndBorrow(cleanupLoc, iterBorrow);
339+
340+
B.createStore(cleanupLoc, iter, iterAddr, StoreOwnershipQualifier::Assign);
341+
342+
B.createBranch(cleanupLoc, loopBB);
343+
}
316344

317345
// the next link in the chain is not unique, so we are done here
318-
B.emitBlock(notUniqueBB);
319-
B.createBranch(cleanupLoc, cleanBB);
346+
{
347+
B.emitBlock(notUniqueBB);
348+
B.createBranch(cleanupLoc, cleanBB);
349+
}
320350

321351
// we reached the end of the chain
322-
B.emitBlock(noneBB);
323-
B.createBranch(cleanupLoc, cleanBB);
352+
{
353+
B.emitBlock(noneBB);
354+
B.createBranch(cleanupLoc, cleanBB);
355+
}
324356

325-
B.emitBlock(cleanBB);
326-
B.createDestroyAddr(cleanupLoc, iterAddr);
327-
B.createDeallocStack(cleanupLoc, iterAddr);
357+
{
358+
B.emitBlock(cleanBB);
359+
B.createDestroyAddr(cleanupLoc, iterAddr);
360+
B.createDeallocStack(cleanupLoc, iterAddr);
361+
}
328362
}
329363

330364
void SILGenFunction::emitClassMemberDestruction(ManagedValue selfValue,
@@ -355,6 +389,10 @@ void SILGenFunction::emitClassMemberDestruction(ManagedValue selfValue,
355389
finishBB);
356390
}
357391

392+
// Before we destroy all fields, we check if any of them are
393+
// recursively the same type as `self`, so we can iteratively
394+
// deinitialize them, to prevent deep recursion and potential
395+
// stack overflows.
358396
llvm::SmallSetVector<VarDecl*, 4> recursiveLinks;
359397
findRecursiveLinks(cd, recursiveLinks);
360398

‎lib/SILGen/SILGenFunction.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -683,6 +683,13 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
683683
/// Generates code to destroy linearly recursive data structures, without
684684
/// building up the call stack.
685685
///
686+
/// Example:
687+
///
688+
/// class Node<A> {
689+
/// let value: A
690+
/// let next: Node<A>?
691+
/// }
692+
///
686693
/// \param selfValue The 'self' value.
687694
/// \param cd The class declaration whose members are being destroyed.
688695
/// \param recursiveLink The property that forms the recursive structure.

‎test/SILGen/deinit_recursive_no_overflow.swift renamed to ‎test/Interpreter/deinit_recursive_no_overflow.swift

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
1-
// RUN: %empty-directory(%t)
2-
// RUN: %target-build-swift %s -o %t/a.out
3-
// RUN: %target-codesign %t/a.out
4-
// RUN: %target-run %t/a.out
1+
// RUN: %target-run-simple-swift
52

63
// REQUIRES: executable_test
74

@@ -20,4 +17,4 @@ for _ in 1...3_000_000 {
2017
func deallocList() {
2118
first = nil
2219
}
23-
deallocList()
20+
deallocList()

‎test/SILGen/deinit_recursive_linear.swift

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,11 @@ class Node {
2323
// CHECK: br [[LOOPBB:bb.*]] //
2424

2525
// CHECK: [[LOOPBB]]:
26-
// CHECK: switch_enum_addr [[ITER]] : $*Optional<Node>, case #Optional.some!enumelt: [[IS_SOME_BB:bb.*]], case #Optional.none!enumelt: [[IS_NONE_BB:bb[0-9]+]]
26+
// CHECK: [[ITER_COPY:%.*]] = load [copy] [[ITER]] : $*Optional<Node>
27+
// CHECK: switch_enum [[ITER_COPY]] : $Optional<Node>, case #Optional.some!enumelt: [[IS_SOME_BB:bb.*]], case #Optional.none!enumelt: [[IS_NONE_BB:bb[0-9]+]]
2728

28-
// CHECK: [[IS_SOME_BB]]:
29+
// CHECK: [[IS_SOME_BB]]([[NODE:%.*]] : @owned $Node):
30+
// CHECK: destroy_value [[NODE]] : $Node
2931
// CHECK: [[IS_UNIQUE:%.*]] = is_unique [[ITER]] : $*Optional<Node>
3032
// CHECK: cond_br [[IS_UNIQUE]], [[IS_UNIQUE_BB:bb.*]], [[NOT_UNIQUE_BB:bb[0-9]*]]
3133

‎test/SILGen/deinit_recursive_linear_generic.swift

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,11 @@ class Node<A, B> {
1818
// CHECK: br [[LOOPBB:bb.*]] //
1919

2020
// CHECK: [[LOOPBB]]:
21-
// CHECK: switch_enum_addr [[ITER]] : $*Optional<Node<A, B>>, case #Optional.some!enumelt: [[IS_SOME_BB:bb.*]], case #Optional.none!enumelt: [[IS_NONE_BB:bb[0-9]+]]
21+
// CHECK: [[ITER_COPY:%.*]] = load [copy] [[ITER]] : $*Optional<Node<A, B>>
22+
// CHECK: switch_enum [[ITER_COPY]] : $Optional<Node<A, B>>, case #Optional.some!enumelt: [[IS_SOME_BB:bb.*]], case #Optional.none!enumelt: [[IS_NONE_BB:bb[0-9]+]]
2223

23-
// CHECK: [[IS_SOME_BB]]:
24+
// CHECK: [[IS_SOME_BB]]([[NODE:%.*]] : @owned $Node<A, B>):
25+
// CHECK: destroy_value [[NODE]] : $Node<A, B>
2426
// CHECK: [[IS_UNIQUE:%.*]] = is_unique [[ITER]] : $*Optional<Node<A, B>>
2527
// CHECK: cond_br [[IS_UNIQUE]], [[IS_UNIQUE_BB:bb.*]], [[NOT_UNIQUE_BB:bb[0-9]*]]
2628

0 commit comments

Comments
 (0)