Skip to content

Commit 8e98bcf

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 2623c4b commit 8e98bcf

File tree

3 files changed

+473
-10
lines changed

3 files changed

+473
-10
lines changed

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

0 commit comments

Comments
 (0)