Skip to content

Commit c5e9fc2

Browse files
committed
[cast-optimizer] Fix a leak in the cast optimizer when we perform load promotion and fail.
Specifically, in this part of the cast optimizer we are trying to optimize casts that can be done in two parts. As an example consider: NSObject -> Array<Any>. In this case, we first cast from NSObject -> NSArray and then try to conditionally bridge to Array<Any> from NSArray. The problem is we did not destroy the NSObject correctly if the first cast failed. I couldn't figure out how to create an actual swift test case that produces this problem since we are pretty conservative about triggering this code path. But in SIL it is pretty easy and in ossa, we trigger the ownership verifier. This is another victory for the ownership verifier! rdar://51753580
1 parent 096d32b commit c5e9fc2

File tree

3 files changed

+1766
-54
lines changed

3 files changed

+1766
-54
lines changed

lib/SILOptimizer/Utils/CastOptimizer.cpp

Lines changed: 50 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -111,15 +111,18 @@ convertObjectToLoadableBridgeableType(SILBuilderWithScope &builder,
111111
return {cast, cast};
112112
}
113113

114+
SILBasicBlock *castSuccessBB =
115+
f->createBasicBlockAfter(dynamicCast.getInstruction()->getParent());
116+
castSuccessBB->createPhiArgument(silBridgedTy, ValueOwnershipKind::Owned);
117+
114118
// If we /are/ conditional and we do not need to bridge the load to the sil,
115119
// then we just create our cast success block and branch from the end of the
116120
// cast instruction block to the cast success block. We leave our insertion
117121
// point in the cast success block since when we return, we are going to
118122
// insert the bridge call/switch there. We return the argument of the cast
119123
// success block as the value to be passed to the bridging function.
120124
if (load->getType() == silBridgedTy) {
121-
SILBasicBlock *castSuccessBB = f->createBasicBlock();
122-
castSuccessBB->createPhiArgument(silBridgedTy, ValueOwnershipKind::Owned);
125+
castSuccessBB->moveAfter(dynamicCast.getInstruction()->getParent());
123126
builder.createBranch(loc, castSuccessBB, load);
124127
builder.setInsertionPoint(castSuccessBB);
125128
return {castSuccessBB->getArgument(0), nullptr};
@@ -132,15 +135,51 @@ convertObjectToLoadableBridgeableType(SILBuilderWithScope &builder,
132135
nullptr, nullptr);
133136
}());
134137

135-
// Ok, we need to perform the full cast optimization. This means that we are
136-
// going to replace the cast terminator in inst_block with a
137-
// checked_cast_br. This in turn means
138-
auto *castSuccessBB = f->createBasicBlock();
139-
castSuccessBB->createPhiArgument(silBridgedTy, ValueOwnershipKind::Owned);
138+
// Now that we have created the failure bb, move our cast success block right
139+
// after the checked_cast_br bb.
140+
castSuccessBB->moveAfter(dynamicCast.getInstruction()->getParent());
140141

142+
// Ok, we need to perform the full cast optimization. This means that we are
143+
// going to replace the cast terminator in inst_block with a checked_cast_br.
141144
auto *ccbi = builder.createCheckedCastBranch(loc, false, load, silBridgedTy,
142145
castSuccessBB, castFailBB);
143146
splitEdge(ccbi, /* EdgeIdx to CastFailBB */ 1);
147+
148+
// Now that we have split the edge to cast fail bb, add the default argument
149+
// for the checked_cast_br. Then we need to handle our error conditions,
150+
// namely we destroy on take_always and otherwise store the value back into
151+
// the memory location that we took it out of.
152+
{
153+
auto *newFailureBlock = ccbi->getFailureBB();
154+
SILValue defaultArg;
155+
if (builder.hasOwnership()) {
156+
defaultArg = newFailureBlock->createPhiArgument(
157+
load->getType(), ValueOwnershipKind::Owned);
158+
} else {
159+
defaultArg = ccbi->getOperand();
160+
}
161+
162+
// This block should be properly terminated already due to our method of
163+
// splitting the failure block, so we can use begin() safely.
164+
SILBuilderWithScope failureBuilder(newFailureBlock->begin());
165+
166+
switch (dynamicCast.getBridgedConsumptionKind()) {
167+
case CastConsumptionKind::TakeAlways:
168+
failureBuilder.emitDestroyValueOperation(loc, defaultArg);
169+
break;
170+
case CastConsumptionKind::TakeOnSuccess:
171+
case CastConsumptionKind::CopyOnSuccess:
172+
// Without ownership, we do not need to consume the taken value.
173+
if (failureBuilder.hasOwnership()) {
174+
failureBuilder.emitStoreValueOperation(loc, defaultArg, src,
175+
StoreOwnershipQualifier::Init);
176+
}
177+
break;
178+
case CastConsumptionKind::BorrowAlways:
179+
llvm_unreachable("this should never occur here");
180+
}
181+
}
182+
144183
builder.setInsertionPoint(castSuccessBB);
145184
return {castSuccessBB->getArgument(0), ccbi};
146185
}
@@ -370,12 +409,13 @@ CastOptimizer::optimizeBridgedObjCToSwiftCast(SILDynamicCastInst dynamicCast) {
370409
// Load from the optional.
371410
auto *SomeDecl = Builder.getASTContext().getOptionalSomeDecl();
372411

373-
SILBasicBlock *BridgeSuccessBB = Inst->getFunction()->createBasicBlock();
412+
auto *BridgeSuccessBB =
413+
Inst->getFunction()->createBasicBlockAfter(Builder.getInsertionBB());
374414
SmallVector<std::pair<EnumElementDecl *, SILBasicBlock *>, 2> CaseBBs;
415+
CaseBBs.emplace_back(SomeDecl, BridgeSuccessBB);
375416
CaseBBs.emplace_back(mod.getASTContext().getOptionalNoneDecl(), FailureBB);
376417

377-
Builder.createSwitchEnumAddr(Loc, InOutOptionalParam, BridgeSuccessBB,
378-
CaseBBs);
418+
Builder.createSwitchEnumAddr(Loc, InOutOptionalParam, nullptr, CaseBBs);
379419

380420
Builder.setInsertionPoint(FailureBB->begin());
381421
Builder.createDeallocStack(Loc, Tmp);

0 commit comments

Comments
 (0)