Skip to content

Commit 5eb830c

Browse files
jckarterjckarter
committed
SIL: Correct handling of bridging casts with address-only types.
We would leak AnyHashable or other bridged address-only value types when turning a take-always cast into a bridging call, since the bridging method takes the value as guaranteed and we didn't arrange to destroy the value after the bridging happened. The code had unnecessarily different paths for indirect and direct arguments, and furthermore, has an untestable path for when the self argument of the bridging method is taken at +1 (when it's currently only ever +0). Join up the direct and indirect logic, and move the handling of differences down to where we introduce retain/releases so that we generate in-memory operations when appropriate. Fixes SR-6465 | rdar://problem/35678523.
1 parent 39dfe07 commit 5eb830c

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)