Skip to content

Commit f09a7e7

Browse files
atrickatrick
authored
Merge pull request #40533 from atrick/fix-deadborrow
Workaround OSSA dead borrow scope bugs
2 parents 36d6a34 + 1f72588 commit f09a7e7

File tree

4 files changed

+179
-19
lines changed

4 files changed

+179
-19
lines changed

‎lib/SIL/Utils/OwnershipUtils.cpp‎

Lines changed: 48 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -178,10 +178,17 @@ bool swift::findInnerTransitiveGuaranteedUses(
178178
break;
179179
}
180180
case OperandOwnership::Borrow:
181-
BorrowingOperand(use).visitExtendedScopeEndingUses([&](Operand *endUse) {
182-
leafUse(endUse);
183-
return true;
184-
});
181+
// FIXME: visitExtendedScopeEndingUses can't return false here once dead
182+
// borrows are disallowed.
183+
if (!BorrowingOperand(use).visitExtendedScopeEndingUses(
184+
[&](Operand *endUse) {
185+
leafUse(endUse);
186+
return true;
187+
})) {
188+
// Special case for dead borrows. This is dangerous because clients
189+
// don't expect a begin_borrow to be in the use list.
190+
leafUse(use);
191+
}
185192
break;
186193
}
187194
}
@@ -275,10 +282,17 @@ bool swift::findInnerTransitiveGuaranteedUsesOfBorrowedValue(
275282
break;
276283
}
277284
case OperandOwnership::Borrow:
278-
BorrowingOperand(use).visitExtendedScopeEndingUses([&](Operand *endUse) {
279-
recordUse(endUse);
280-
return true;
281-
});
285+
// FIXME: visitExtendedScopeEndingUses can't return false here once dead
286+
// borrows are disallowed.
287+
if (!BorrowingOperand(use).visitExtendedScopeEndingUses(
288+
[&](Operand *endUse) {
289+
recordUse(endUse);
290+
return true;
291+
})) {
292+
// Special case for dead borrows. This is dangerous because clients
293+
// don't expect a begin_borrow to be in the use list.
294+
recordUse(use);
295+
}
282296
break;
283297
}
284298
}
@@ -427,21 +441,29 @@ bool BorrowingOperand::visitScopeEndingUses(
427441
switch (kind) {
428442
case BorrowingOperandKind::Invalid:
429443
llvm_unreachable("Using invalid case");
430-
case BorrowingOperandKind::BeginBorrow:
444+
case BorrowingOperandKind::BeginBorrow: {
445+
bool deadBorrow = true;
431446
for (auto *use : cast<BeginBorrowInst>(op->getUser())->getUses()) {
432447
if (use->isLifetimeEnding()) {
448+
deadBorrow = false;
433449
if (!func(use))
434450
return false;
435451
}
436452
}
437-
return true;
453+
// FIXME: special case for dead borrows. This is dangerous because clients
454+
// only expect visitScopeEndingUses to return false if the visitor returned
455+
// false.
456+
return !deadBorrow;
457+
}
438458
case BorrowingOperandKind::BeginApply: {
459+
bool deadApply = true;
439460
auto *user = cast<BeginApplyInst>(op->getUser());
440461
for (auto *use : user->getTokenResult()->getUses()) {
462+
deadApply = false;
441463
if (!func(use))
442464
return false;
443465
}
444-
return true;
466+
return !deadApply;
445467
}
446468
// These are instantaneous borrow scopes so there aren't any special end
447469
// scope instructions.
@@ -450,12 +472,16 @@ bool BorrowingOperand::visitScopeEndingUses(
450472
case BorrowingOperandKind::Yield:
451473
return true;
452474
case BorrowingOperandKind::Branch: {
475+
bool deadBranch = true;
453476
auto *br = cast<BranchInst>(op->getUser());
454-
for (auto *use : br->getArgForOperand(op)->getUses())
455-
if (use->isLifetimeEnding())
477+
for (auto *use : br->getArgForOperand(op)->getUses()) {
478+
if (use->isLifetimeEnding()) {
479+
deadBranch = false;
456480
if (!func(use))
457481
return false;
458-
return true;
482+
}
483+
}
484+
return !deadBranch;
459485
}
460486
}
461487
llvm_unreachable("Covered switch isn't covered");
@@ -522,10 +548,15 @@ BorrowedValue BorrowingOperand::getBorrowIntroducingUserResult() {
522548
void BorrowingOperand::getImplicitUses(
523549
SmallVectorImpl<Operand *> &foundUses,
524550
std::function<void(Operand *)> *errorFunction) const {
525-
visitScopeEndingUses([&](Operand *op) {
526-
foundUses.push_back(op);
551+
// FIXME: this visitScopeEndingUses should never return false once dead
552+
// borrows are disallowed.
553+
if (!visitScopeEndingUses([&](Operand *endOp) {
554+
foundUses.push_back(endOp);
527555
return true;
528-
});
556+
})) {
557+
// Special-case for dead borrows.
558+
foundUses.push_back(op);
559+
}
529560
}
530561

531562
//===----------------------------------------------------------------------===//

‎lib/SILOptimizer/Utils/CanonicalizeBorrowScope.cpp‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -327,13 +327,18 @@ class FindBorrowScopeUses {
327327
// For borrows, record the scope-ending instructions to outer use
328328
// points. Note: The logic in filterOuterBorrowUseInsts that checks
329329
// whether a borrow scope is an outer use must visit the same set of uses.
330-
borrowingOper.visitExtendedScopeEndingUses([&](Operand *endBorrow) {
330+
//
331+
// FIXME: visitExtendedScopeEndingUses can't return false here once dead
332+
// borrows are disallowed.
333+
if (!borrowingOper.visitExtendedScopeEndingUses([&](Operand *endBorrow) {
331334
auto *endInst = endBorrow->getUser();
332335
if (!isUserInLiveOutBlock(endInst)) {
333336
useInsts.insert(endInst);
334337
}
335338
return true;
336-
});
339+
})) {
340+
useInsts.insert(user);
341+
}
337342
}
338343
return true;
339344
}

‎test/SILOptimizer/copy_propagation.sil‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -817,3 +817,22 @@ bb4:
817817
return %v : $()
818818
}
819819

820+
// Test a dead begin_borrow (with no scope ending uses). Make sure
821+
// copy-propagation doesn't end the lifetime before the dead borrow.
822+
//
823+
// CHECK-LABEL: sil hidden [ossa] @testDeadBorrow : $@convention(thin) (@owned C) -> () {
824+
// CHECK: bb0(%0 : @owned $C):
825+
// CHECK: copy_value %0 : $C
826+
// CHECK: destroy_value
827+
// CHECK: copy_value %0 : $C
828+
// CHECK: begin_borrow
829+
// CHECK: unreachable
830+
// CHECK-LABEL: } // end sil function 'testDeadBorrow'
831+
sil hidden [ossa] @testDeadBorrow : $@convention(thin) (@owned C) -> () {
832+
bb0(%0 : @owned $C):
833+
%1 = copy_value %0 : $C
834+
destroy_value %1 : $C
835+
%6 = copy_value %0 : $C
836+
%7 = begin_borrow %6 : $C
837+
unreachable
838+
}

‎test/SILOptimizer/semantic-arc-opts.sil‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import Builtin
99
//////////////////
1010

1111
typealias AnyObject = Builtin.AnyObject
12+
protocol Error {}
1213

1314
enum MyNever {}
1415
enum FakeOptional<T> {
@@ -1525,3 +1526,107 @@ bb3(%7 : @owned $FakeOptional<Builtin.NativeObject>):
15251526
%9999 = tuple()
15261527
return %9999 : $()
15271528
}
1529+
1530+
// SemanticARCOptVisitor::performGuaranteedCopyValueOptimization should not optimize this case.
1531+
//
1532+
// CHECK-LABEL: sil [ossa] @testDeadInterleavedBorrow : $@convention(c) (@owned FakeOptional<AnyObject>) -> () {
1533+
// CHECK: [[B:%.*]] = begin_borrow [lexical] %0 : $FakeOptional<AnyObject>
1534+
// CHECK: copy_value
1535+
// CHECK: switch_enum
1536+
// CHECK: bb{{.*}}([[COPY:%.*]] : @owned $AnyObject):
1537+
// CHECK: begin_borrow [lexical] [[COPY]] : $AnyObject
1538+
// CHECK: end_borrow [[B]] : $FakeOptional<AnyObject>
1539+
// CHECK: end_borrow
1540+
// CHECK: destroy_value [[COPY]] : $AnyObject
1541+
// CHECK-LABEL: } // end sil function 'testDeadInterleavedBorrow'
1542+
sil [ossa] @testDeadInterleavedBorrow : $@convention(c) (@owned FakeOptional<AnyObject>) -> () {
1543+
bb0(%opt : @owned $FakeOptional<AnyObject>):
1544+
%opt_borrow = begin_borrow [lexical] %opt : $FakeOptional<AnyObject>
1545+
%opt_borrow_copy = copy_value %opt_borrow : $FakeOptional<AnyObject>
1546+
switch_enum %opt_borrow_copy : $FakeOptional<AnyObject>, case #FakeOptional.some!enumelt: some, case #FakeOptional.none!enumelt: none
1547+
1548+
some(%obj : @owned $AnyObject):
1549+
%_borrow = begin_borrow [lexical] %obj : $AnyObject
1550+
end_borrow %opt_borrow : $FakeOptional<AnyObject>
1551+
end_borrow %_borrow : $AnyObject
1552+
destroy_value %obj : $AnyObject
1553+
br die
1554+
1555+
die:
1556+
unreachable
1557+
1558+
none:
1559+
end_borrow %opt_borrow : $FakeOptional<AnyObject>
1560+
br exit
1561+
1562+
exit:
1563+
destroy_value %opt : $FakeOptional<AnyObject>
1564+
%res = tuple ()
1565+
return %res : $()
1566+
}
1567+
1568+
// SemanticARCOptVisitor::performGuaranteedCopyValueOptimization should not optimize this case.
1569+
//
1570+
// CHECK-LABEL: sil [ossa] @testDeadAndAliveInterleavedBorrow : $@convention(c) (@owned FakeOptional<AnyObject>) -> () {
1571+
// CHECK: [[B:%.*]] = begin_borrow [lexical] %0 : $FakeOptional<AnyObject>
1572+
// CHECK: copy_value
1573+
// CHECK: switch_enum
1574+
//
1575+
// CHECK: bb{{.*}}([[COPY:%.*]] : @owned $AnyObject):
1576+
// CHECK: [[BB:%.*]] = begin_borrow [lexical] [[COPY]] : $AnyObject
1577+
//
1578+
// CHECK: try_apply undef
1579+
//
1580+
// CHECK: [[RESULT:%.*]] = apply undef(%{{.*}}) : $@convention(method) (@guaranteed AnyObject) -> @owned FakeOptional<AnyObject>
1581+
// CHECK: switch_enum [[RESULT]] : $FakeOptional<AnyObject>, case #FakeOptional.some!enumelt: {{.*}}, case #FakeOptional.none!enumelt: [[NONEBB:bb[0-9]?]]
1582+
//
1583+
// CHECK: [[NONEBB]]:
1584+
// CHECK: end_borrow [[B]] : $FakeOptional<AnyObject>
1585+
// CHECK: end_borrow [[BB]] : $AnyObject
1586+
// CHECK: destroy_value [[COPY]] : $AnyObject
1587+
// CHECK: destroy_value %0 : $FakeOptional<AnyObject>
1588+
// CHECK-LABEL: } // end sil function 'testDeadAndAliveInterleavedBorrow'
1589+
sil [ossa] @testDeadAndAliveInterleavedBorrow : $@convention(c) (@owned FakeOptional<AnyObject>) -> () {
1590+
bb0(%opt : @owned $FakeOptional<AnyObject>):
1591+
%opt_borrow = begin_borrow [lexical] %opt : $FakeOptional<AnyObject>
1592+
%opt_borrow_copy = copy_value %opt_borrow : $FakeOptional<AnyObject>
1593+
switch_enum %opt_borrow_copy : $FakeOptional<AnyObject>, case #FakeOptional.some!enumelt: some, case #FakeOptional.none!enumelt: none
1594+
1595+
some(%obj : @owned $AnyObject):
1596+
%obj_borrow = begin_borrow [lexical] %obj : $AnyObject
1597+
%obj_borrow_copy = copy_value %obj_borrow : $AnyObject
1598+
try_apply undef(%obj_borrow_copy) : $@convention(method) (@owned AnyObject) -> (@owned AnyObject, @error Error), normal good, error bad
1599+
1600+
good(%obj2 : @owned $AnyObject):
1601+
%obj3 = apply undef(%obj2) : $@convention(method) (@guaranteed AnyObject) -> @owned FakeOptional<AnyObject>
1602+
destroy_value %obj2 : $AnyObject
1603+
switch_enum %obj3 : $FakeOptional<AnyObject>, case #FakeOptional.some!enumelt: good_some, case #FakeOptional.none!enumelt: good_none
1604+
1605+
good_some(%obj4 : @owned $AnyObject):
1606+
destroy_value %obj4 : $AnyObject
1607+
end_borrow %obj_borrow : $AnyObject
1608+
destroy_value %obj : $AnyObject
1609+
br exit
1610+
1611+
good_none:
1612+
end_borrow %opt_borrow : $FakeOptional<AnyObject>
1613+
end_borrow %obj_borrow : $AnyObject
1614+
destroy_value %obj : $AnyObject
1615+
destroy_value %opt : $FakeOptional<AnyObject>
1616+
br die
1617+
1618+
bad(%error1 : @owned $Error):
1619+
br die
1620+
1621+
die:
1622+
unreachable
1623+
1624+
none:
1625+
br exit
1626+
1627+
exit:
1628+
end_borrow %opt_borrow : $FakeOptional<AnyObject>
1629+
destroy_value %opt : $FakeOptional<AnyObject>
1630+
%res = tuple ()
1631+
return %res : $()
1632+
}

0 commit comments

Comments
 (0)