Skip to content

Commit 16f5e64

Browse files
jckarterjckarter
authored
Merge pull request #13101 from jckarter/address-only-bridged-cast-leak
SIL: Correct handling of bridging casts with address-only types.
2 parents fa1d07c + 5eb830c commit 16f5e64

File tree

3 files changed

+182
-49
lines changed

3 files changed

+182
-49
lines changed

‎lib/SILOptimizer/Utils/Local.cpp

Lines changed: 79 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1687,79 +1687,81 @@ optimizeBridgedSwiftToObjCCast(SILInstruction *Inst,
16871687
// and the inserted conversion function.
16881688
bool needRetainBeforeCall = false;
16891689
bool needReleaseAfterCall = false;
1690-
bool needReleaseInSucc = false;
1690+
bool needReleaseInSuccess = false;
16911691
switch (ParamTypes[0].getConvention()) {
16921692
case ParameterConvention::Direct_Guaranteed:
1693-
assert(!AddressOnlyType &&
1694-
"AddressOnlyType with Direct_Guaranteed is not supported");
1693+
case ParameterConvention::Indirect_In_Guaranteed:
16951694
switch (ConsumptionKind) {
1696-
case CastConsumptionKind::TakeAlways:
1697-
needReleaseAfterCall = true;
1698-
break;
1699-
case CastConsumptionKind::TakeOnSuccess:
1700-
needReleaseInSucc = true;
1701-
break;
1702-
case CastConsumptionKind::CopyOnSuccess:
1703-
// Conservatively insert a retain/release pair around the conversion
1704-
// function because the conversion function could decrement the
1705-
// (global) reference count of the source object.
1706-
//
1707-
// %src = load %global_var
1708-
// apply %conversion_func(@guaranteed %src)
1709-
//
1710-
// sil conversion_func {
1711-
// %old_value = load %global_var
1712-
// store %something_else, %global_var
1713-
// strong_release %old_value
1714-
// }
1715-
needRetainBeforeCall = true;
1716-
needReleaseAfterCall = true;
1717-
break;
1695+
case CastConsumptionKind::TakeAlways:
1696+
needReleaseAfterCall = true;
1697+
break;
1698+
case CastConsumptionKind::TakeOnSuccess:
1699+
needReleaseInSuccess = true;
1700+
break;
1701+
case CastConsumptionKind::CopyOnSuccess:
1702+
// Conservatively insert a retain/release pair around the conversion
1703+
// function because the conversion function could decrement the
1704+
// (global) reference count of the source object.
1705+
//
1706+
// %src = load %global_var
1707+
// apply %conversion_func(@guaranteed %src)
1708+
//
1709+
// sil conversion_func {
1710+
// %old_value = load %global_var
1711+
// store %something_else, %global_var
1712+
// strong_release %old_value
1713+
// }
1714+
needRetainBeforeCall = true;
1715+
needReleaseAfterCall = true;
1716+
break;
17181717
}
17191718
break;
17201719
case ParameterConvention::Direct_Owned:
1721-
// The Direct_Owned case is only handled for completeness. Currently this
1720+
case ParameterConvention::Indirect_In:
1721+
case ParameterConvention::Indirect_In_Constant:
1722+
// Currently this
17221723
// cannot appear, because the _bridgeToObjectiveC protocol witness method
17231724
// always receives the this pointer (= the source) as guaranteed.
1724-
assert(!AddressOnlyType &&
1725-
"AddressOnlyType with Direct_Owned is not supported");
1725+
// If it became possible (perhaps with the advent of ownership and
1726+
// explicit +1 annotations), the implementation should look something
1727+
// like this:
1728+
/*
17261729
switch (ConsumptionKind) {
17271730
case CastConsumptionKind::TakeAlways:
17281731
break;
17291732
case CastConsumptionKind::TakeOnSuccess:
17301733
needRetainBeforeCall = true;
1731-
needReleaseInSucc = true;
1734+
needReleaseInSuccess = true;
17321735
break;
17331736
case CastConsumptionKind::CopyOnSuccess:
17341737
needRetainBeforeCall = true;
17351738
break;
17361739
}
17371740
break;
1741+
*/
1742+
llvm_unreachable("this should never happen so is currently untestable");
17381743
case ParameterConvention::Direct_Unowned:
17391744
assert(!AddressOnlyType &&
17401745
"AddressOnlyType with Direct_Unowned is not supported");
17411746
break;
1742-
case ParameterConvention::Indirect_In_Guaranteed:
1743-
// Source as-is, we don't need to copy it due to guarantee
1744-
break;
1745-
case ParameterConvention::Indirect_In_Constant:
1746-
case ParameterConvention::Indirect_In: {
1747-
assert(substConv.isSILIndirect(ParamTypes[0])
1748-
&& "unsupported convention for bridging conversion");
1749-
// Need to make a copy of the source, can be changed in ObjC
1750-
auto BridgeStack = Builder.createAllocStack(Loc, Src->getType());
1751-
Builder.createCopyAddr(Loc, Src, BridgeStack, IsNotTake,
1752-
IsInitialization);
1753-
break;
1754-
}
17551747
case ParameterConvention::Indirect_Inout:
17561748
case ParameterConvention::Indirect_InoutAliasable:
17571749
// TODO handle remaining indirect argument types
17581750
return nullptr;
17591751
}
17601752

1761-
if (needRetainBeforeCall)
1762-
Builder.createRetainValue(Loc, Src, Builder.getDefaultAtomicity());
1753+
bool needStackAllocatedTemporary = false;
1754+
if (needRetainBeforeCall) {
1755+
if (AddressOnlyType) {
1756+
needStackAllocatedTemporary = true;
1757+
auto NewSrc = Builder.createAllocStack(Loc, Src->getType());
1758+
Builder.createCopyAddr(Loc, Src, NewSrc,
1759+
IsNotTake, IsInitialization);
1760+
Src = NewSrc;
1761+
} else {
1762+
Builder.createRetainValue(Loc, Src, Builder.getDefaultAtomicity());
1763+
}
1764+
}
17631765

17641766
SmallVector<Substitution, 4> Subs;
17651767
if (auto *Sig = Source->getAnyNominal()->getGenericSignature())
@@ -1768,12 +1770,41 @@ optimizeBridgedSwiftToObjCCast(SILInstruction *Inst,
17681770
// Generate a code to invoke the bridging function.
17691771
auto *NewAI = Builder.createApply(Loc, FnRef, Subs, Src, false);
17701772

1773+
auto releaseSrc = [&](SILBuilder &Builder) {
1774+
if (AddressOnlyType) {
1775+
Builder.createDestroyAddr(Loc, Src);
1776+
} else {
1777+
Builder.createReleaseValue(Loc, Src, Builder.getDefaultAtomicity());
1778+
}
1779+
};
1780+
1781+
Optional<SILBuilder> SuccBuilder;
1782+
if (needReleaseInSuccess || needStackAllocatedTemporary)
1783+
SuccBuilder.emplace(SuccessBB->begin());
1784+
17711785
if (needReleaseAfterCall) {
1772-
Builder.createReleaseValue(Loc, Src, Builder.getDefaultAtomicity());
1773-
} else if (needReleaseInSucc) {
1774-
SILBuilder SuccBuilder(SuccessBB->begin());
1775-
SuccBuilder.createReleaseValue(Loc, Src, SuccBuilder.getDefaultAtomicity());
1786+
releaseSrc(Builder);
1787+
} else if (needReleaseInSuccess) {
1788+
if (SuccessBB) {
1789+
releaseSrc(*SuccBuilder);
1790+
} else {
1791+
// For an unconditional cast, success is the only defined path
1792+
releaseSrc(Builder);
1793+
}
1794+
}
1795+
1796+
// Pop the temporary stack slot for a copied temporary.
1797+
if (needStackAllocatedTemporary) {
1798+
assert((bool)SuccessBB == (bool)FailureBB);
1799+
if (SuccessBB) {
1800+
SuccBuilder->createDeallocStack(Loc, Src);
1801+
SILBuilder FailBuilder(FailureBB->begin());
1802+
FailBuilder.createDeallocStack(Loc, Src);
1803+
} else {
1804+
Builder.createDeallocStack(Loc, Src);
1805+
}
17761806
}
1807+
17771808
SILInstruction *NewI = NewAI;
17781809

17791810
if (Dest) {

‎test/SILOptimizer/bridged_casts_folding.sil

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
// RUN: %target-swift-frontend -O -enable-sil-ownership -emit-sil %s | %FileCheck %s
2+
3+
// REQUIRES: objc_interop
4+
5+
sil_stage raw
6+
7+
import Swift
8+
import Foundation
9+
10+
class NSObjectSubclass : NSObject {}
11+
sil_vtable NSObjectSubclass {}
12+
13+
// CHECK-LABEL: sil @anyhashable_cast_unconditional
14+
// CHECK: [[BRIDGED:%.*]] = apply {{.*}}(%0)
15+
// CHECK-NEXT: destroy_addr %0
16+
// CHECK-NEXT: [[CAST:%.*]] = unconditional_checked_cast [[BRIDGED]] : $NSObject to $NSObjectSubclass
17+
sil @anyhashable_cast_unconditional : $@convention(thin) (@in AnyHashable) -> @owned NSObjectSubclass {
18+
entry(%0 : @trivial $*AnyHashable):
19+
%1 = alloc_stack $NSObjectSubclass
20+
unconditional_checked_cast_addr AnyHashable in %0 : $*AnyHashable
21+
to NSObjectSubclass in %1 : $*NSObjectSubclass
22+
%3 = load [take] %1 : $*NSObjectSubclass
23+
dealloc_stack %1 : $*NSObjectSubclass
24+
return %3 : $NSObjectSubclass
25+
}
26+
27+
// CHECK-LABEL: sil @anyhashable_cast_take_always
28+
// CHECK: [[BRIDGED:%.*]] = apply {{.*}}(%0)
29+
// CHECK-NEXT: destroy_addr %0
30+
// CHECK-NEXT: checked_cast_br [[BRIDGED]] : $NSObject to $NSObjectSubclass, [[YES:bb[0-9]+]], [[NO:bb[0-9]+]]
31+
sil @anyhashable_cast_take_always : $@convention(thin) (@in AnyHashable, @owned NSObjectSubclass) -> @owned NSObjectSubclass {
32+
entry(%0 : @trivial $*AnyHashable, %1 : @owned $NSObjectSubclass):
33+
%2 = alloc_stack $NSObjectSubclass
34+
checked_cast_addr_br take_always AnyHashable in %0 : $*AnyHashable
35+
to NSObjectSubclass in %2 : $*NSObjectSubclass, bb1, bb2
36+
37+
bb1:
38+
%4 = load [take] %2 : $*NSObjectSubclass
39+
destroy_value %1 : $NSObjectSubclass
40+
br bb3(%4 : $NSObjectSubclass)
41+
42+
bb2:
43+
br bb3(%1 : $NSObjectSubclass)
44+
45+
bb3(%8 : @owned $NSObjectSubclass):
46+
dealloc_stack %2 : $*NSObjectSubclass
47+
return %8 : $NSObjectSubclass
48+
}
49+
50+
// CHECK-LABEL: sil @anyhashable_cast_take_on_success
51+
// CHECK: [[BRIDGED:%.*]] = apply {{.*}}(%0)
52+
// CHECK-NEXT: checked_cast_br [[BRIDGED]] : $NSObject to $NSObjectSubclass, [[YES:bb[0-9]+]], [[NO:bb[0-9]+]]
53+
// CHECK: [[YES]]{{.*}}:
54+
// CHECK-NEXT: destroy_addr %0
55+
sil @anyhashable_cast_take_on_success : $@convention(thin) (@in AnyHashable, @owned NSObjectSubclass) -> @owned NSObjectSubclass {
56+
entry(%0 : @trivial $*AnyHashable, %1 : @owned $NSObjectSubclass):
57+
%2 = alloc_stack $NSObjectSubclass
58+
checked_cast_addr_br take_on_success AnyHashable in %0 : $*AnyHashable
59+
to NSObjectSubclass in %2 : $*NSObjectSubclass, bb1, bb2
60+
61+
bb1:
62+
%4 = load [take] %2 : $*NSObjectSubclass
63+
destroy_value %1 : $NSObjectSubclass
64+
br bb3(%4 : $NSObjectSubclass)
65+
66+
bb2:
67+
destroy_addr %0 : $*AnyHashable
68+
br bb3(%1 : $NSObjectSubclass)
69+
70+
bb3(%8 : @owned $NSObjectSubclass):
71+
dealloc_stack %2 : $*NSObjectSubclass
72+
return %8 : $NSObjectSubclass
73+
}
74+
75+
// CHECK-LABEL: sil @anyhashable_cast_copy_on_success
76+
// CHECK: copy_addr %0 to [initialization] [[TEMP:%.*]] : $*AnyHashable
77+
// CHECK-NEXT: [[BRIDGED:%.*]] = apply {{.*}}([[TEMP]])
78+
// CHECK-NEXT: destroy_addr [[TEMP]]
79+
// CHECK-NEXT: checked_cast_br [[BRIDGED]] : $NSObject to $NSObjectSubclass, [[YES:bb[0-9]+]], [[NO:bb[0-9]+]]
80+
// CHECK: [[YES]]{{.*}}:
81+
// CHECK-NEXT: dealloc_stack [[TEMP]]
82+
// CHECK: [[NO]]{{.*}}:
83+
// CHECK-NEXT: dealloc_stack [[TEMP]]
84+
sil @anyhashable_cast_copy_on_success : $@convention(thin) (@in_guaranteed AnyHashable, @owned NSObjectSubclass) -> @owned NSObjectSubclass {
85+
entry(%0 : @trivial $*AnyHashable, %1 : @owned $NSObjectSubclass):
86+
%2 = alloc_stack $NSObjectSubclass
87+
checked_cast_addr_br copy_on_success AnyHashable in %0 : $*AnyHashable
88+
to NSObjectSubclass in %2 : $*NSObjectSubclass, bb1, bb2
89+
90+
bb1:
91+
%4 = load [take] %2 : $*NSObjectSubclass
92+
destroy_value %1 : $NSObjectSubclass
93+
br bb3(%4 : $NSObjectSubclass)
94+
95+
bb2:
96+
br bb3(%1 : $NSObjectSubclass)
97+
98+
bb3(%8 : @owned $NSObjectSubclass):
99+
dealloc_stack %2 : $*NSObjectSubclass
100+
return %8 : $NSObjectSubclass
101+
}

‎test/SILOptimizer/bridged_casts_folding.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -904,7 +904,8 @@ var anyHashable: AnyHashable = 0
904904
// CHECK-LABEL: _T021bridged_casts_folding29testUncondCastSwiftToSubclassAA08NSObjectI0CyF
905905
// CHECK: [[GLOBAL:%[0-9]+]] = global_addr @_T021bridged_casts_folding11anyHashables03AnyE0Vv
906906
// CHECK: function_ref @_T0s11AnyHashableV10FoundationE19_bridgeToObjectiveCSo8NSObjectCyF
907-
// CHECK-NEXT: apply
907+
// CHECK-NEXT: apply %3(%1)
908+
// CHECK-NEXT: destroy_addr %1
908909
// CHECK-NEXT: unconditional_checked_cast {{%.*}} : $NSObject to $NSObjectSubclass
909910
@inline(never)
910911
public func testUncondCastSwiftToSubclass() -> NSObjectSubclass {

0 commit comments

Comments
 (0)