Skip to content

Commit 350109f

Browse files
authored
[silgen] Fix lifetime management of tuple elements so we don't leak (#39600)
* Revert "[silgen] Ensure that the outer cleanup is emitted along failure paths when initializing sub-tuple patterns" This reverts commit be922b9. By adding some extra scopes here we are triggering some broken behavior in a bunch of projects. I am going to see if I can do another fix for this. That being said in the short term, we are reverting to unblock those projects. rdar://83770295 (cherry picked from commit 49bc96d) * [silgen] When initializing tuples in SILGen, first evaluate all tuple elts and then perform sub-initialization. Previsouly we were evaluating a tuple elt and then performing the relevant sub-initialization. The problem is that a sub-initialization can invoke code that could perform an early exit cleanup. So any later tuple-elements that may need to be cleaned up along such path will not have had their cleanups initialized, resulting in a leak along such paths. With this commit, we instead evaluate all of the tuple elements and only them perform the sub-initialization ensuring that any early exits clean up all of the tuple elements. rdar://83770295 (cherry picked from commit f641808)
1 parent cec5425 commit 350109f

File tree

4 files changed

+44
-48
lines changed

4 files changed

+44
-48
lines changed

lib/SILGen/SILGenBuilder.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -854,6 +854,16 @@ void SILGenBuilder::emitDestructureValueOperation(
854854
}
855855
}
856856

857+
void SILGenBuilder::emitDestructureValueOperation(
858+
SILLocation loc, ManagedValue value,
859+
SmallVectorImpl<ManagedValue> &destructuredValues) {
860+
CleanupCloner cloner(*this, value);
861+
emitDestructureValueOperation(
862+
loc, value.forward(SGF), [&](unsigned index, SILValue subValue) {
863+
destructuredValues.push_back(cloner.clone(subValue));
864+
});
865+
}
866+
857867
ManagedValue SILGenBuilder::createProjectBox(SILLocation loc, ManagedValue mv,
858868
unsigned index) {
859869
auto *pbi = createProjectBox(loc, mv.getValue(), index);

lib/SILGen/SILGenBuilder.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,9 @@ class SILGenBuilder : public SILBuilder {
372372
void emitDestructureValueOperation(
373373
SILLocation loc, ManagedValue value,
374374
function_ref<void(unsigned, ManagedValue)> func);
375+
void emitDestructureValueOperation(
376+
SILLocation loc, ManagedValue value,
377+
SmallVectorImpl<ManagedValue> &destructuredValues);
375378

376379
using SILBuilder::createProjectBox;
377380
ManagedValue createProjectBox(SILLocation loc, ManagedValue mv,

lib/SILGen/SILGenDecl.cpp

Lines changed: 29 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -39,59 +39,42 @@ using namespace Lowering;
3939
void Initialization::_anchor() {}
4040
void SILDebuggerClient::anchor() {}
4141

42-
static void copyOrInitValueIntoHelper(
43-
SILGenFunction &SGF, SILLocation loc, ManagedValue value, bool isInit,
44-
ArrayRef<InitializationPtr> subInitializations,
45-
llvm::function_ref<ManagedValue(ManagedValue, unsigned, SILType)> func) {
46-
auto sourceType = value.getType().castTo<TupleType>();
47-
auto sourceSILType = value.getType();
48-
for (unsigned i = 0, e = sourceType->getNumElements(); i != e; ++i) {
49-
SILType fieldTy = sourceSILType.getTupleElementType(i);
50-
ManagedValue elt = func(value, i, fieldTy);
51-
subInitializations[i]->copyOrInitValueInto(SGF, loc, elt, isInit);
52-
subInitializations[i]->finishInitialization(SGF);
53-
}
54-
}
55-
5642
void TupleInitialization::copyOrInitValueInto(SILGenFunction &SGF,
5743
SILLocation loc,
5844
ManagedValue value, bool isInit) {
45+
// Process all values before initialization all at once to ensure all cleanups
46+
// are setup on all tuple elements before a potential early exit.
47+
SmallVector<ManagedValue, 8> destructuredValues;
48+
5949
// In the object case, emit a destructure operation and return.
6050
if (value.getType().isObject()) {
61-
return SGF.B.emitDestructureValueOperation(
62-
loc, value, [&](unsigned i, ManagedValue subValue) {
63-
auto &subInit = SubInitializations[i];
64-
subInit->copyOrInitValueInto(SGF, loc, subValue, isInit);
65-
subInit->finishInitialization(SGF);
66-
});
51+
SGF.B.emitDestructureValueOperation(loc, value, destructuredValues);
52+
} else {
53+
// In the address case, we forward the underlying value and store it
54+
// into memory and then create a +1 cleanup. since we assume here
55+
// that we have a +1 value since we are forwarding into memory.
56+
assert(value.isPlusOne(SGF) && "Can not store a +0 value into memory?!");
57+
CleanupCloner cloner(SGF, value);
58+
SILValue v = value.forward(SGF);
59+
60+
auto sourceType = value.getType().castTo<TupleType>();
61+
auto sourceSILType = value.getType();
62+
for (unsigned i : range(sourceType->getNumElements())) {
63+
SILType fieldTy = sourceSILType.getTupleElementType(i);
64+
SILValue elt = SGF.B.createTupleElementAddr(loc, v, i, fieldTy);
65+
if (!fieldTy.isAddressOnly(SGF.F)) {
66+
elt = SGF.B.emitLoadValueOperation(loc, elt,
67+
LoadOwnershipQualifier::Take);
68+
}
69+
destructuredValues.push_back(cloner.clone(elt));
70+
}
6771
}
6872

69-
// In the address case, we forward the underlying value and store it
70-
// into memory and then create a +1 cleanup. since we assume here
71-
// that we have a +1 value since we are forwarding into memory.
72-
//
73-
// In order to ensure that we properly clean up along any failure paths, we
74-
// need to mark value as being persistently active. We then unforward it once
75-
// we are done.
76-
assert(value.isPlusOne(SGF) && "Can not store a +0 value into memory?!");
77-
CleanupStateRestorationScope valueScope(SGF.Cleanups);
78-
if (value.hasCleanup())
79-
valueScope.pushCleanupState(value.getCleanup(),
80-
CleanupState::PersistentlyActive);
81-
copyOrInitValueIntoHelper(
82-
SGF, loc, value, isInit, SubInitializations,
83-
[&](ManagedValue aggregate, unsigned i,
84-
SILType fieldType) -> ManagedValue {
85-
ManagedValue elt =
86-
SGF.B.createTupleElementAddr(loc, value, i, fieldType);
87-
if (!fieldType.isAddressOnly(SGF.F)) {
88-
return SGF.B.createLoadTake(loc, elt);
89-
}
90-
91-
return SGF.emitManagedRValueWithCleanup(elt.getValue());
92-
});
93-
std::move(valueScope).pop();
94-
value.forward(SGF);
73+
for (unsigned i : indices(destructuredValues)) {
74+
SubInitializations[i]->copyOrInitValueInto(SGF, loc, destructuredValues[i],
75+
isInit);
76+
SubInitializations[i]->finishInitialization(SGF);
77+
}
9578
}
9679

9780
void TupleInitialization::finishUninitialized(SILGenFunction &SGF) {

test/SILGen/indirect_enum.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -448,8 +448,8 @@ func guardTreeB<T>(_ tree: TreeB<T>) {
448448
// CHECK: [[TUPLE_ADDR:%.*]] = project_box [[BOX]]
449449
// CHECK: copy_addr [[TUPLE_ADDR]] to [initialization] [[TUPLE_COPY:%.*]] :
450450
// CHECK: [[L_COPY:%.*]] = tuple_element_addr [[TUPLE_COPY]]
451-
// CHECK: copy_addr [take] [[L_COPY]] to [initialization] [[L]]
452451
// CHECK: [[R_COPY:%.*]] = tuple_element_addr [[TUPLE_COPY]]
452+
// CHECK: copy_addr [take] [[L_COPY]] to [initialization] [[L]]
453453
// CHECK: copy_addr [take] [[R_COPY]] to [initialization] [[R]]
454454
// CHECK: destroy_value [[BOX]]
455455
guard case .Branch(left: let l, right: let r) = tree else { return }
@@ -492,8 +492,8 @@ func guardTreeB<T>(_ tree: TreeB<T>) {
492492
// CHECK: [[TUPLE_ADDR:%.*]] = project_box [[BOX]]
493493
// CHECK: copy_addr [[TUPLE_ADDR]] to [initialization] [[TUPLE_COPY:%.*]] :
494494
// CHECK: [[L_COPY:%.*]] = tuple_element_addr [[TUPLE_COPY]]
495-
// CHECK: copy_addr [take] [[L_COPY]] to [initialization] [[L]]
496495
// CHECK: [[R_COPY:%.*]] = tuple_element_addr [[TUPLE_COPY]]
496+
// CHECK: copy_addr [take] [[L_COPY]] to [initialization] [[L]]
497497
// CHECK: copy_addr [take] [[R_COPY]] to [initialization] [[R]]
498498
// CHECK: destroy_value [[BOX]]
499499
// CHECK: destroy_addr [[R]]

0 commit comments

Comments
 (0)