Skip to content

Commit 1eafced

Browse files
nate-chandlernate-chandler
committed
[AllocBoxToStack] Don't destroy in dead-ends.
It is valid to leak a value on paths into dead-end regions. Specifically, it is valid to leak an `alloc_box`. Thus, "final releases" in dead-end regions may not destroy the box and consequently may not release its contents. Therefore it's invalid to lower such final releases to `dealloc_stack`s, let alone `destroy_addr`s. The in-general invalidity of that transformation results in miscompiling whenever a box is leaked and its projected address is used after such final releases. Fix this by not treating final releases as boundary markers of the `alloc_box` and not lowering them to `destroy_addr`s and `dealloc_stack`s. rdar://158149082
1 parent 74528ee commit 1eafced

File tree

4 files changed

+462
-11
lines changed

4 files changed

+462
-11
lines changed

‎SwiftCompilerSources/Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -362,7 +362,28 @@ private func createAllocStack(for allocBox: AllocBoxInst, flags: Flags, _ contex
362362
for destroy in getFinalDestroys(of: allocBox, context) {
363363
let loc = allocBox.location.asCleanup.withScope(of: destroy.location)
364364
Builder.insert(after: destroy, location: loc, context) { builder in
365-
if !unboxedType.isTrivial(in: allocBox.parentFunction), !(destroy is DeallocBoxInst) {
365+
if !(destroy is DeallocBoxInst),
366+
context.deadEndBlocks.isDeadEnd(destroy.parentBlock),
367+
!isInLoop(block: destroy.parentBlock, context) {
368+
// "Last" releases in dead-end regions may not actually destroy the box
369+
// and consequently may not actually release the stored value. That's
370+
// because values (including boxes) may be leaked along paths into
371+
// dead-end regions. Thus it is invalid to lower such final releases of
372+
// the box to destroy_addr's/dealloc_box's of the stack-promoted storage.
373+
//
374+
// There is one exception: if the alloc_box is in a dead-end loop. In
375+
// that case SIL invariants require that the final releases actually
376+
// destroy the box; otherwise, a box would leak once per loop. To check
377+
// for this, it is sufficient check that the LastRelease is in a dead-end
378+
// loop: if the alloc_box is not in that loop, then the entire loop is in
379+
// the live range, so no release within the loop would be a "final
380+
// release".
381+
//
382+
// None of this applies to dealloc_box instructions which always destroy
383+
// the box.
384+
return
385+
}
386+
if !unboxedType.isTrivial(in: allocBox.parentFunction), !(destroy is DeallocBoxInst) {
366387
builder.createDestroyAddr(address: stackLocation)
367388
}
368389
if let dbi = destroy as? DeallocBoxInst, dbi.isDeadEnd {

‎lib/SILOptimizer/Transforms/AllocBoxToStack.cpp‎

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
#include "swift/SIL/SILArgument.h"
2626
#include "swift/SIL/SILBuilder.h"
2727
#include "swift/SIL/SILCloner.h"
28+
#include "swift/SILOptimizer/Analysis/DeadEndBlocksAnalysis.h"
29+
#include "swift/SILOptimizer/Analysis/LoopAnalysis.h"
2830
#include "swift/SILOptimizer/PassManager/Passes.h"
2931
#include "swift/SILOptimizer/PassManager/Transforms.h"
3032
#include "swift/SILOptimizer/Utils/InstOptUtils.h"
@@ -601,7 +603,9 @@ static void hoistMarkUnresolvedNonCopyableValueInsts(
601603

602604
/// rewriteAllocBoxAsAllocStack - Replace uses of the alloc_box with a
603605
/// new alloc_stack, but do not delete the alloc_box yet.
604-
static bool rewriteAllocBoxAsAllocStack(AllocBoxInst *ABI) {
606+
static bool rewriteAllocBoxAsAllocStack(AllocBoxInst *ABI,
607+
DeadEndBlocksAnalysis &deba,
608+
SILLoopAnalysis &la) {
605609
LLVM_DEBUG(llvm::dbgs() << "*** Promoting alloc_box to stack: " << *ABI);
606610

607611
SILValue HeapBox = ABI;
@@ -693,9 +697,31 @@ static bool rewriteAllocBoxAsAllocStack(AllocBoxInst *ABI) {
693697
ABI->getBoxType(), ABI->getModule().Types, 0));
694698
auto Loc = CleanupLocation(ABI->getLoc());
695699

700+
auto *deb = deba.get(ABI->getFunction());
696701
for (auto LastRelease : FinalReleases) {
702+
auto *dbi = dyn_cast<DeallocBoxInst>(LastRelease);
703+
if (!dbi && deb->isDeadEnd(LastRelease->getParent()) &&
704+
!la.get(ABI->getFunction())->getLoopFor(LastRelease->getParent())) {
705+
// "Last" releases in dead-end regions may not actually destroy the box
706+
// and consequently may not actually release the stored value. That's
707+
// because values (including boxes) may be leaked along paths into
708+
// dead-end regions. Thus it is invalid to lower such final releases of
709+
// the box to destroy_addr's/dealloc_box's of the stack-promoted storage.
710+
//
711+
// There is one exception: if the alloc_box is in a dead-end loop. In
712+
// that case SIL invariants require that the final releases actually
713+
// destroy the box; otherwise, a box would leak once per loop. To check
714+
// for this, it is sufficient check that the LastRelease is in a dead-end
715+
// loop: if the alloc_box is not in that loop, then the entire loop is in
716+
// the live range, so no release within the loop would be a "final
717+
// release".
718+
//
719+
// None of this applies to dealloc_box instructions which always destroy
720+
// the box.
721+
continue;
722+
}
697723
SILBuilderWithScope Builder(LastRelease);
698-
if (!isa<DeallocBoxInst>(LastRelease)&& !Lowering.isTrivial()) {
724+
if (!dbi && !Lowering.isTrivial()) {
699725
// If we have a mark_unresolved_non_copyable_value use of our stack box,
700726
// we want to destroy that.
701727
SILValue valueToDestroy = StackBox;
@@ -709,7 +735,6 @@ static bool rewriteAllocBoxAsAllocStack(AllocBoxInst *ABI) {
709735
// instruction we found that isn't an explicit dealloc_box.
710736
Builder.emitDestroyAddrAndFold(Loc, valueToDestroy);
711737
}
712-
auto *dbi = dyn_cast<DeallocBoxInst>(LastRelease);
713738
if (dbi && dbi->isDeadEnd()) {
714739
// Don't bother to create dealloc_stack instructions in dead-ends.
715740
continue;
@@ -1265,7 +1290,9 @@ static void rewriteApplySites(AllocBoxToStackState &pass) {
12651290

12661291
/// Clone closure bodies and rewrite partial applies. Returns the number of
12671292
/// alloc_box allocations promoted.
1268-
static unsigned rewritePromotedBoxes(AllocBoxToStackState &pass) {
1293+
static unsigned rewritePromotedBoxes(AllocBoxToStackState &pass,
1294+
DeadEndBlocksAnalysis &deba,
1295+
SILLoopAnalysis &la) {
12691296
// First we'll rewrite any ApplySite that we can to remove
12701297
// the box container pointer from the operands.
12711298
rewriteApplySites(pass);
@@ -1274,7 +1301,7 @@ static unsigned rewritePromotedBoxes(AllocBoxToStackState &pass) {
12741301
auto rend = pass.Promotable.rend();
12751302
for (auto I = pass.Promotable.rbegin(); I != rend; ++I) {
12761303
auto *ABI = *I;
1277-
if (rewriteAllocBoxAsAllocStack(ABI)) {
1304+
if (rewriteAllocBoxAsAllocStack(ABI, deba, la)) {
12781305
++Count;
12791306
ABI->eraseFromParent();
12801307
}
@@ -1299,7 +1326,9 @@ class AllocBoxToStack : public SILFunctionTransform {
12991326
}
13001327

13011328
if (!pass.Promotable.empty()) {
1302-
auto Count = rewritePromotedBoxes(pass);
1329+
auto *deba = getAnalysis<DeadEndBlocksAnalysis>();
1330+
auto *la = getAnalysis<SILLoopAnalysis>();
1331+
auto Count = rewritePromotedBoxes(pass, *deba, *la);
13031332
NumStackPromoted += Count;
13041333
if (Count) {
13051334
if (StackNesting::fixNesting(getFunction()) == StackNesting::Changes::CFG)

‎test/SILOptimizer/allocbox_to_stack.sil‎

Lines changed: 200 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ struct Bool {
1414

1515
protocol Error {}
1616

17+
class C {}
18+
1719
// CHECK-LABEL: sil @simple_promotion
1820
sil @simple_promotion : $(Int) -> Int {
1921
bb0(%0 : $Int):
@@ -900,7 +902,6 @@ bb2:
900902
// CHECK: bb1:
901903
// CHECK-NEXT: [[STACK2:%[0-9]+]] = alloc_stack $Bool
902904
// CHECK-NEXT: dealloc_stack [[STACK2]]
903-
// CHECK-NEXT: dealloc_stack [[BOX]]
904905
// CHECK-NEXT: unreachable
905906
// CHECK: bb2:
906907
// CHECK: store {{%[0-9]+}}
@@ -1058,10 +1059,8 @@ bb3:
10581059
// CHECK-NEXT: [[AI:%[0-9]+]] = alloc_stack $Int
10591060
// CHECK-NEXT: cond_br
10601061
// CHECK: bb1:
1061-
// CHECK-NEXT: dealloc_stack [[AI]]
10621062
// CHECK-NEXT: br bb3
10631063
// CHECK: bb2:
1064-
// CHECK-NEXT: dealloc_stack [[AI]]
10651064
// CHECK-NEXT: br bb3
10661065
// CHECK: bb3:
10671066
// CHECK-NEXT: store {{%[0-9]+}}
@@ -1225,3 +1224,201 @@ bb0:
12251224
destroy_value %box : ${ var Builtin.Int32 }
12261225
return %value : $Builtin.Int32
12271226
}
1227+
1228+
sil @getC : $@convention(thin) () -> (@owned C)
1229+
1230+
// CHECK-LABEL: sil @leak_to_inf_loop_1 : {{.*}} {
1231+
// CHECK-NOT: destroy_addr
1232+
// CHECK-NOT: dealloc_stack
1233+
// CHECK-LABEL: } // end sil function 'leak_to_inf_loop_1'
1234+
sil @leak_to_inf_loop_1 : $@convention(thin) (@owned C) -> () {
1235+
entry(%c : $C):
1236+
%box = alloc_box ${ var C }
1237+
%c_addr = project_box %box, 0
1238+
store %c to %c_addr
1239+
strong_retain %box
1240+
strong_release %box
1241+
br loop
1242+
1243+
loop:
1244+
%getC = function_ref @getC : $@convention(thin) () -> (@owned C)
1245+
%c2 = apply %getC() : $@convention(thin) () -> (@owned C)
1246+
%c_old = load %c_addr
1247+
store %c2 to %c_addr
1248+
release_value %c_old
1249+
br loop
1250+
}
1251+
1252+
// CHECK-LABEL: sil @leak_to_inf_loop_2 : {{.*}} {
1253+
// CHECK: cond_br undef, [[EXIT:bb[0-9]+]], [[TO_LOOP:bb[0-9]+]]
1254+
// CHECK: [[EXIT]]
1255+
// CHECK: destroy_addr
1256+
// CHECK: dealloc_stack
1257+
// CHECK: [[TO_LOOP]]
1258+
// CHECK-NOT: destroy_addr
1259+
// CHECK-NOT: dealloc_stack
1260+
// CHECK-LABEL: } // end sil function 'leak_to_inf_loop_2'
1261+
sil @leak_to_inf_loop_2 : $@convention(thin) (@owned C) -> () {
1262+
entry(%c : $C):
1263+
%box = alloc_box ${ var C }
1264+
%c_addr = project_box %box, 0
1265+
store %c to %c_addr
1266+
strong_retain %box
1267+
strong_release %box
1268+
cond_br undef, exit, to_loop
1269+
1270+
exit:
1271+
strong_release %box
1272+
return undef : $()
1273+
1274+
to_loop:
1275+
strong_retain %box
1276+
strong_release %box
1277+
br loop
1278+
1279+
loop:
1280+
%getC = function_ref @getC : $@convention(thin) () -> (@owned C)
1281+
%c2 = apply %getC() : $@convention(thin) () -> (@owned C)
1282+
%c_old = load %c_addr
1283+
store %c2 to %c_addr
1284+
release_value %c_old
1285+
br loop
1286+
}
1287+
1288+
// CHECK-LABEL: sil @leak_to_inf_loop_3 : {{.*}} {
1289+
// CHECK-NOT: destroy_addr
1290+
// CHECK-NOT: dealloc_stack
1291+
// CHECK-LABEL: } // end sil function 'leak_to_inf_loop_3'
1292+
sil @leak_to_inf_loop_3 : $@convention(thin) (@owned C) -> () {
1293+
entry(%c : $C):
1294+
%box = alloc_box ${ var C }
1295+
%c_addr = project_box %box, 0
1296+
store %c to %c_addr
1297+
br to_loop
1298+
1299+
to_loop:
1300+
strong_retain %box
1301+
strong_release %box
1302+
br loop
1303+
1304+
loop:
1305+
%getC = function_ref @getC : $@convention(thin) () -> (@owned C)
1306+
%c2 = apply %getC() : $@convention(thin) () -> (@owned C)
1307+
%c_old = load %c_addr
1308+
store %c2 to %c_addr
1309+
release_value %c_old
1310+
br loop
1311+
}
1312+
1313+
// CHECK-LABEL: sil @dealloc_box_before_loop
1314+
// CHECK: dealloc_stack
1315+
// CHECK-LABEL: } // end sil function 'dealloc_box_before_loop'
1316+
sil @dealloc_box_before_loop : $@convention(thin) (@owned C) -> () {
1317+
entry(%c : $C):
1318+
%box = alloc_box ${ var C }
1319+
%c_addr = project_box %box, 0
1320+
store %c to %c_addr
1321+
cond_br undef, exit, to_loop
1322+
1323+
exit:
1324+
strong_release %box
1325+
return undef : $()
1326+
1327+
to_loop:
1328+
dealloc_box %box
1329+
br loop
1330+
1331+
loop:
1332+
br loop
1333+
}
1334+
1335+
// CHECK-LABEL: sil @alloc_box_in_deadend_loop
1336+
// CHECK: alloc_stack
1337+
// CHECK: dealloc_stack
1338+
// CHECK-LABEL: } // end sil function 'alloc_box_in_deadend_loop'
1339+
sil @alloc_box_in_deadend_loop : $@convention(thin) () -> () {
1340+
entry:
1341+
br loop
1342+
1343+
loop:
1344+
%box = alloc_box ${ var C }
1345+
%c_addr = project_box %box, 0
1346+
%getC = function_ref @getC : $@convention(thin) () -> (@owned C)
1347+
%c = apply %getC() : $@convention(thin) () -> (@owned C)
1348+
store %c to %c_addr
1349+
strong_release %box
1350+
br loop
1351+
}
1352+
1353+
// CHECK-LABEL: sil @alloc_box_in_exiting_loop
1354+
// CHECK: br [[EXITING_LOOP:bb[0-9]+]]
1355+
// CHECK: [[EXITING_LOOP]]:
1356+
// CHECK: alloc_stack
1357+
// CHECK: cond_br undef, [[EXIT:bb[0-9]+]], [[LATCH:bb[0-9]+]]
1358+
// CHECK: [[LATCH]]:
1359+
// CHECK: cond_br undef, [[BACKEDGE:bb[0-9]+]], [[INFINITE_LOOP:bb[0-9]+]]
1360+
// CHECK: [[BACKEDGE]]:
1361+
// CHECK: dealloc_stack
1362+
// CHECK: br [[EXITING_LOOP]]
1363+
// CHECK: [[EXIT]]:
1364+
// CHECK: dealloc_stack
1365+
// CHECK: [[INFINITE_LOOP]]:
1366+
// CHECK-NOT: dealloc_stack
1367+
// CHECK-LABEL: } // end sil function 'alloc_box_in_exiting_loop'
1368+
sil @alloc_box_in_exiting_loop : $@convention(thin) () -> () {
1369+
entry:
1370+
br exiting_loop
1371+
1372+
exiting_loop:
1373+
%box = alloc_box ${ var C }
1374+
%c_addr = project_box %box, 0
1375+
cond_br undef, exit, latch
1376+
1377+
latch:
1378+
cond_br undef, backedge, infinite_loop
1379+
1380+
backedge:
1381+
strong_release %box
1382+
br exiting_loop
1383+
1384+
exit:
1385+
strong_release %box
1386+
return undef : $()
1387+
1388+
infinite_loop:
1389+
%getC = function_ref @getC : $@convention(thin) () -> (@owned C)
1390+
%c = apply %getC() : $@convention(thin) () -> (@owned C)
1391+
store %c to %c_addr
1392+
strong_retain %box
1393+
strong_release %box
1394+
br infinite_loop
1395+
}
1396+
1397+
// CHECK-LABEL: sil @alloc_box_in_deadend_loop_with_another_deadend
1398+
// CHECK: br [[LOOP:bb[0-9]+]]
1399+
// CHECK: [[LOOP]]:
1400+
// CHECK: alloc_stack
1401+
// CHECK: cond_br undef, [[BACKEDGE:bb[0-9]+]], [[DIE:bb[0-9]+]]
1402+
// CHECK: [[BACKEDGE]]:
1403+
// CHECK: dealloc_stack
1404+
// CHECK: br [[LOOP]]
1405+
// CHECK: [[DIE]]:
1406+
// CHECK-NOT: dealloc_stack
1407+
// CHECK-LABEL: } // end sil function 'alloc_box_in_deadend_loop_with_another_deadend'
1408+
sil @alloc_box_in_deadend_loop_with_another_deadend : $@convention(thin) () -> () {
1409+
entry:
1410+
br loop
1411+
1412+
loop:
1413+
%box = alloc_box ${ var C }
1414+
cond_br undef, backedge, die
1415+
1416+
backedge:
1417+
strong_release %box
1418+
br loop
1419+
1420+
die:
1421+
strong_retain %box
1422+
strong_release %box
1423+
unreachable
1424+
}

0 commit comments

Comments
 (0)