Skip to content

Commit 1072bb6

Browse files
eecksteineeckstein
committed
SILCombine: fix a miscompile caused by dead-apply elimination
SILCombine ended up moving a strong_release past a dealloc_ref. fixes https://bugs.swift.org/browse/SR-9627 rdar://problem/47153896
1 parent b446d6b commit 1072bb6

File tree

5 files changed

+158
-0
lines changed

5 files changed

+158
-0
lines changed

‎include/swift/SILOptimizer/Utils/Local.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,9 @@ class ValueLifetimeAnalysis {
282282
return LiveBlocks.count(BB) && BB != DefValue->getParent();
283283
}
284284

285+
/// Checks if there is a dealloc_ref inside the value's live range.
286+
bool containsDeallocRef(const Frontier &Frontier);
287+
285288
/// For debug dumping.
286289
void dump() const;
287290

‎lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -547,6 +547,13 @@ bool SILCombiner::eraseApply(FullApplySite FAS, const UserListTy &Users) {
547547
// Compute the places where we have to insert release-instructions for the
548548
// owned arguments. This must not be done before the result of the
549549
// apply is destroyed. Therefore we compute the lifetime of the apply-result.
550+
551+
// TODO: this is not required anymore when we have ownership SIL. But with
552+
// the current SIL it can happen that the retain of a parameter is moved
553+
// _after_ the apply.
554+
// When we have ownership SIL we can just destroy the parameters at the apply
555+
// location.
556+
550557
ValueLifetimeAnalysis VLA(FAS.getInstruction(), Users);
551558
ValueLifetimeAnalysis::Frontier Frontier;
552559
if (Users.empty()) {
@@ -556,6 +563,12 @@ bool SILCombiner::eraseApply(FullApplySite FAS, const UserListTy &Users) {
556563
} else {
557564
if (!VLA.computeFrontier(Frontier, ValueLifetimeAnalysis::DontModifyCFG))
558565
return false;
566+
// As we are extending the lifetimes of owned parameters, we have to make
567+
// sure that no dealloc_ref instructions are within this extended liferange.
568+
// It could be that the dealloc_ref is deallocating a parameter and then
569+
// we would have a release after the dealloc.
570+
if (VLA.containsDeallocRef(Frontier))
571+
return false;
559572
}
560573

561574
// Release and destroy any owned or in-arguments.

‎lib/SILOptimizer/Utils/Local.cpp

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1369,6 +1369,44 @@ bool ValueLifetimeAnalysis::isWithinLifetime(SILInstruction *Inst) {
13691369
llvm_unreachable("Expected to find use of value in block!");
13701370
}
13711371

1372+
// Searches \p BB backwards from the instruction before \p FrontierInst
1373+
// to the beginning of the list and returns true if we find a dealloc_ref
1374+
// /before/ we find \p DefValue (the instruction that defines our target value).
1375+
static bool blockContainsDeallocRef(SILBasicBlock *BB, SILInstruction *DefValue,
1376+
SILInstruction *FrontierInst) {
1377+
SILBasicBlock::reverse_iterator End = BB->rend();
1378+
SILBasicBlock::reverse_iterator Iter = FrontierInst->getReverseIterator();
1379+
for (++Iter; Iter != End; ++Iter) {
1380+
SILInstruction *I = &*Iter;
1381+
if (isa<DeallocRefInst>(I))
1382+
return true;
1383+
if (I == DefValue)
1384+
return false;
1385+
}
1386+
return false;
1387+
}
1388+
1389+
bool ValueLifetimeAnalysis::containsDeallocRef(const Frontier &Frontier) {
1390+
SmallPtrSet<SILBasicBlock *, 8> FrontierBlocks;
1391+
// Search in live blocks where the value is not alive until the end of the
1392+
// block, i.e. the live range is terminated by a frontier instruction.
1393+
for (SILInstruction *FrontierInst : Frontier) {
1394+
SILBasicBlock *BB = FrontierInst->getParent();
1395+
if (blockContainsDeallocRef(BB, DefValue, FrontierInst))
1396+
return true;
1397+
FrontierBlocks.insert(BB);
1398+
}
1399+
// Search in all other live blocks where the value is alive until the end of
1400+
// the block.
1401+
for (SILBasicBlock *BB : LiveBlocks) {
1402+
if (FrontierBlocks.count(BB) == 0) {
1403+
if (blockContainsDeallocRef(BB, DefValue, BB->getTerminator()))
1404+
return true;
1405+
}
1406+
}
1407+
return false;
1408+
}
1409+
13721410
void ValueLifetimeAnalysis::dump() const {
13731411
llvm::errs() << "lifetime of def: " << *DefValue;
13741412
for (SILInstruction *Use : UserSet) {

‎test/SILOptimizer/sil_combine.sil

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1980,6 +1980,89 @@ bb3:
19801980
return %0 : $B
19811981
}
19821982

1983+
// CHECK-LABEL: sil @dont_delete_readonly_function_dealloc_ref_simple
1984+
// CHECK: apply
1985+
// CHECK-NEXT: dealloc_ref
1986+
// CHECK: return
1987+
sil @dont_delete_readonly_function_dealloc_ref_simple : $@convention(thin) () -> () {
1988+
bb0:
1989+
%3 = alloc_ref [stack] $B
1990+
%f = function_ref @readonly_owned : $@convention(thin) (@owned B) -> @owned B
1991+
%r = apply %f(%3) : $@convention(thin) (@owned B) -> @owned B
1992+
dealloc_ref [stack] %3 : $B
1993+
strong_release %r : $B
1994+
%10 = tuple ()
1995+
return %10 : $()
1996+
}
1997+
1998+
// CHECK-LABEL: sil @dont_delete_readonly_function_dealloc_ref_multibb
1999+
// CHECK: apply
2000+
// CHECK: dealloc_ref
2001+
// CHECK-NEXT: strong_release
2002+
// CHECK: dealloc_ref
2003+
// CHECK-NEXT: strong_release
2004+
// CHECK: return
2005+
sil @dont_delete_readonly_function_dealloc_ref_multibb : $@convention(thin) () -> () {
2006+
bb0:
2007+
%3 = alloc_ref [stack] $B
2008+
%f = function_ref @readonly_owned : $@convention(thin) (@owned B) -> @owned B
2009+
%r = apply %f(%3) : $@convention(thin) (@owned B) -> @owned B
2010+
cond_br undef, bb1, bb2
2011+
2012+
bb1:
2013+
dealloc_ref [stack] %3 : $B
2014+
strong_release %r : $B
2015+
br bb3
2016+
2017+
bb2:
2018+
dealloc_ref [stack] %3 : $B
2019+
strong_release %r : $B
2020+
br bb3
2021+
2022+
bb3:
2023+
%10 = tuple ()
2024+
return %10 : $()
2025+
}
2026+
2027+
// CHECK-LABEL: sil @delete_readonly_function_dealloc_ref_simple
2028+
// CHECK-NOT: apply
2029+
// CHECK: return
2030+
sil @delete_readonly_function_dealloc_ref_simple : $@convention(thin) () -> () {
2031+
bb0:
2032+
%3 = alloc_ref [stack] $B
2033+
%f = function_ref @readonly_owned : $@convention(thin) (@owned B) -> @owned B
2034+
%r = apply %f(%3) : $@convention(thin) (@owned B) -> @owned B
2035+
strong_release %r : $B
2036+
dealloc_ref [stack] %3 : $B
2037+
%10 = tuple ()
2038+
return %10 : $()
2039+
}
2040+
2041+
// CHECK-LABEL: sil @delete_readonly_function_dealloc_ref_multibb
2042+
// CHECK-NOT: apply
2043+
// CHECK: return
2044+
sil @delete_readonly_function_dealloc_ref_multibb : $@convention(thin) () -> () {
2045+
bb0:
2046+
%3 = alloc_ref [stack] $B
2047+
%f = function_ref @readonly_owned : $@convention(thin) (@owned B) -> @owned B
2048+
%r = apply %f(%3) : $@convention(thin) (@owned B) -> @owned B
2049+
cond_br undef, bb1, bb2
2050+
2051+
bb1:
2052+
strong_release %r : $B
2053+
dealloc_ref [stack] %3 : $B
2054+
br bb3
2055+
2056+
bb2:
2057+
strong_release %r : $B
2058+
dealloc_ref [stack] %3 : $B
2059+
br bb3
2060+
2061+
bb3:
2062+
%10 = tuple ()
2063+
return %10 : $()
2064+
}
2065+
19832066
//CHECK-LABEL: sil @test_delete_readonly_try_apply
19842067
//CHECK: bb0(%0 : $ZZZ):
19852068
//CHECK-NEXT: [[IL:%[0-9]+]] = integer_literal $Builtin.Int1, 0

‎test/SILOptimizer/silcombine_runtime_crash.swift

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-build-swift -O %s -o %t/a.out
3+
// RUN: %target-run %t/a.out | %FileCheck %s
4+
5+
// REQUIRES: executable_test
6+
7+
// This is an end-to-end test for SR-9627.
8+
9+
@inline(never)
10+
func save(value: Double?) {
11+
var params: [[String : Any]] = [["a": 0]]
12+
params = [[
13+
"b": 0,
14+
"c": value.map({ String.init($0) }) as Any
15+
]]
16+
}
17+
18+
save(value: 0)
19+
20+
// CHECK: ok
21+
print("ok")

0 commit comments

Comments
 (0)