Skip to content

Commit 5491c4e

Browse files
gottesmmgottesmm
authored
Merge pull request #25074 from gottesmm/swift-5.1-branch-rdar51093557
[5.1][cast-opt] Fix miscompile when we tried to optimize take_on_success that resulted in invalid IR being emitted.
2 parents 3fcdacf + 852fb32 commit 5491c4e

File tree

2 files changed

+251
-16
lines changed

2 files changed

+251
-16
lines changed

‎lib/SILOptimizer/Utils/CastOptimizer.cpp

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -228,26 +228,30 @@ CastOptimizer::optimizeBridgedObjCToSwiftCast(SILDynamicCastInst dynamicCast) {
228228
// TODO: Is it safe to just eliminate the initial retain?
229229
Builder.createReleaseValue(Loc, srcOp, Builder.getDefaultAtomicity());
230230

231-
// If the source of a cast should be destroyed, emit a release.
231+
// If we have an unconditional_checked_cast_addr, return early. We do not need
232+
// to handle any conditional code.
232233
if (isa<UnconditionalCheckedCastAddrInst>(Inst)) {
234+
// Destroy the source value as unconditional_checked_cast_addr would.
233235
Builder.createReleaseValue(Loc, srcOp, Builder.getDefaultAtomicity());
236+
EraseInstAction(Inst);
237+
return (newI) ? newI : AI;
234238
}
235239

236-
if (auto *CCABI = dyn_cast<CheckedCastAddrBranchInst>(Inst)) {
237-
switch (CCABI->getConsumptionKind()) {
238-
case CastConsumptionKind::TakeAlways:
239-
Builder.createReleaseValue(Loc, srcOp, Builder.getDefaultAtomicity());
240-
break;
241-
case CastConsumptionKind::TakeOnSuccess:
242-
// Insert a release in the success BB.
243-
Builder.setInsertionPoint(SuccessBB->begin());
244-
Builder.createReleaseValue(Loc, srcOp, Builder.getDefaultAtomicity());
245-
break;
246-
case CastConsumptionKind::BorrowAlways:
247-
llvm_unreachable("checked_cast_addr_br never has BorrowAlways");
248-
case CastConsumptionKind::CopyOnSuccess:
249-
break;
250-
}
240+
auto *CCABI = cast<CheckedCastAddrBranchInst>(Inst);
241+
switch (CCABI->getConsumptionKind()) {
242+
case CastConsumptionKind::TakeAlways:
243+
Builder.createReleaseValue(Loc, srcOp, Builder.getDefaultAtomicity());
244+
break;
245+
case CastConsumptionKind::TakeOnSuccess: {
246+
// Insert a release in the success BB.
247+
SILBuilderWithScope SuccessBuilder(SuccessBB->begin());
248+
SuccessBuilder.emitDestroyValueOperation(Loc, srcOp);
249+
break;
250+
}
251+
case CastConsumptionKind::BorrowAlways:
252+
llvm_unreachable("checked_cast_addr_br never has BorrowAlways");
253+
case CastConsumptionKind::CopyOnSuccess:
254+
break;
251255
}
252256

253257
// Results should be checked in case we process a conditional

‎test/SILOptimizer/constant_propagation_objc.sil

Lines changed: 231 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,231 @@
1+
// RUN: %target-sil-opt -enable-sil-verify-all %s -diagnostic-constant-propagation | %FileCheck %s
2+
// RUN: %target-sil-opt -enable-sil-verify-all %s -performance-constant-propagation | %FileCheck %s
3+
4+
// REQUIRES: objc_interop
5+
6+
sil_stage canonical
7+
8+
import Swift
9+
import Foundation
10+
import Builtin
11+
12+
sil @$ss11AnyHashableVyABxcSHRzlufC : $@convention(method) <τ_0_0 where τ_0_0 : Hashable> (@in τ_0_0, @thin AnyHashable.Type) -> @out AnyHashable
13+
14+
sil @guaranteed_swift_array_user : $@convention(thin) <τ_0_0> (@guaranteed Array<τ_0_0>) -> ()
15+
16+
// CHECK-LABEL: sil @array_downcast_copyonsuccess : $@convention(thin) (@guaranteed NSArray) -> () {
17+
// CHECK: bb0([[ARG:%.*]] : $NSArray):
18+
// CHECK: [[INPUT:%.*]] = alloc_stack $NSArray
19+
// CHECK: retain_value [[ARG]]
20+
// CHECK: store [[ARG]] to [[INPUT]]
21+
// CHECK: [[OUTPUT:%.*]] = alloc_stack $Array<String>
22+
// CHECK: [[INPUT_VALUE:%.*]] = load [[INPUT]]
23+
// CHECK: br [[BRIDGE_BB:bb[0-9]+]]([[INPUT_VALUE]] :
24+
//
25+
// CHECK: [[SUCCESS_BB:bb[0-9]+]]:
26+
// CHECK: [[SUCCESS_VAL:%.*]] = load [[OUTPUT]]
27+
// CHECK: [[CAST_RESULT:%.*]] = apply {{%.*}}<String>([[SUCCESS_VAL]])
28+
// CHECK-NEXT: release_value [[SUCCESS_VAL]]
29+
// CHECK-NEXT: dealloc_stack [[OUTPUT]]
30+
// CHECK-NEXT: destroy_addr [[INPUT]]
31+
// CHECK-NEXT: dealloc_stack [[INPUT]]
32+
// CHECK-NEXT: br [[EXIT_BB:bb[0-9]+]]
33+
//
34+
// CHECK: [[FAIL_BB:bb[0-9]+]]:
35+
// CHECK-NEXT: dealloc_stack [[CAST_TMP:%.*]]
36+
// CHECK-NEXT: br [[NEXT:bb[0-9]+]]
37+
//
38+
// CHECK: [[NEXT]]:
39+
// CHECK-NEXT: dealloc_stack [[OUTPUT]]
40+
// CHECK-NEXT: destroy_addr [[INPUT]]
41+
// CHECK-NEXT: dealloc_stack [[INPUT]]
42+
// CHECK-NEXT: br [[EXIT_BB]]
43+
//
44+
// CHECK: [[EXIT_BB]]:
45+
// CHECK: return
46+
//
47+
// CHECK: [[BRIDGE_BB]]([[INPUT_VALUE:%.*]] : $NSArray):
48+
// CHECK: [[CAST_TMP:%.*]] = alloc_stack $Optional<Array<String>>
49+
// CHECK: retain_value [[INPUT_VALUE]]
50+
// CHECK: apply {{%.*}}<Array<String>>([[CAST_TMP]], [[INPUT_VALUE]],
51+
// CHECK: release_value [[INPUT_VALUE]]
52+
// CHECK: switch_enum_addr [[CAST_TMP]] : $*Optional<Array<String>>, case #Optional.none!enumelt: [[FAIL_BB]], default [[SUCCESS_TRAMPOLINE_BB:bb[0-9]+]]
53+
//
54+
// CHECK: [[SUCCESS_TRAMPOLINE_BB]]:
55+
// CHECK: [[PROJ_ENUM:%.*]] = unchecked_take_enum_data_addr [[CAST_TMP]]
56+
// CHECK: copy_addr [take] [[PROJ_ENUM]] to [initialization] [[OUTPUT]]
57+
// CHECK: dealloc_stack [[CAST_TMP]]
58+
// CHECK: br [[SUCCESS_BB]]
59+
// CHECK: } // end sil function 'array_downcast_copyonsuccess'
60+
sil @array_downcast_copyonsuccess : $@convention(thin) (@guaranteed NSArray) -> () {
61+
bb0(%0 : $NSArray):
62+
%4 = alloc_stack $NSArray
63+
retain_value %0 : $NSArray
64+
store %0 to %4 : $*NSArray
65+
%7 = alloc_stack $Array<String>
66+
checked_cast_addr_br copy_on_success NSArray in %4 : $*NSArray to Array<String> in %7 : $*Array<String>, bb2, bb3
67+
68+
bb2:
69+
%9 = load %7 : $*Array<String>
70+
%10 = function_ref @guaranteed_swift_array_user : $@convention(thin) <τ_0_0> (@guaranteed Array<τ_0_0>) -> ()
71+
apply %10<String>(%9) : $@convention(thin) <τ_0_0> (@guaranteed Array<τ_0_0>) -> ()
72+
release_value %9 : $Array<String>
73+
dealloc_stack %7 : $*Array<String>
74+
destroy_addr %4 : $*NSArray
75+
dealloc_stack %4 : $*NSArray
76+
br bb4
77+
78+
bb3:
79+
dealloc_stack %7 : $*Array<String>
80+
destroy_addr %4 : $*NSArray
81+
dealloc_stack %4 : $*NSArray
82+
br bb4
83+
84+
bb4:
85+
%9999 = tuple()
86+
return %9999 : $()
87+
}
88+
89+
// CHECK-LABEL: sil @array_downcast_takeonsuccess : $@convention(thin) (@guaranteed NSArray) -> () {
90+
// CHECK: bb0([[ARG:%.*]] : $NSArray):
91+
// CHECK: [[INPUT:%.*]] = alloc_stack $NSArray
92+
// CHECK: retain_value [[ARG]]
93+
// CHECK: store [[ARG]] to [[INPUT]]
94+
// CHECK: [[OUTPUT:%.*]] = alloc_stack $Array<String>
95+
// CHECK: [[INPUT_VALUE:%.*]] = load [[INPUT]]
96+
// CHECK: br [[BRIDGE_BB:bb[0-9]+]]([[INPUT_VALUE]] :
97+
//
98+
// CHECK: [[SUCCESS_BB:bb[0-9]+]]:
99+
// CHECK: strong_release [[INPUT_VALUE:%.*]] :
100+
// CHECK: [[SUCCESS_VAL:%.*]] = load [[OUTPUT]]
101+
// CHECK: [[CAST_RESULT:%.*]] = apply {{%.*}}<String>([[SUCCESS_VAL]])
102+
// CHECK-NEXT: release_value [[SUCCESS_VAL]]
103+
// CHECK-NEXT: dealloc_stack [[OUTPUT]]
104+
// CHECK-NEXT: dealloc_stack [[INPUT]]
105+
// CHECK-NEXT: br [[EXIT_BB:bb[0-9]+]]
106+
//
107+
// CHECK: [[FAIL_BB:bb[0-9]+]]:
108+
// CHECK-NEXT: dealloc_stack [[CAST_TMP:%.*]]
109+
// CHECK-NEXT: br [[FAIL_NEXT:bb[0-9]+]]
110+
//
111+
// CHECK: [[FAIL_NEXT]]:
112+
// CHECK-NEXT: dealloc_stack [[OUTPUT]]
113+
// CHECK-NEXT: destroy_addr [[INPUT]]
114+
// CHECK-NEXT: dealloc_stack [[INPUT]]
115+
// CHECK-NEXT: br [[EXIT_BB]]
116+
//
117+
// CHECK: [[EXIT_BB]]:
118+
// CHECK: return
119+
//
120+
// CHECK: [[BRIDGE_BB]]([[INPUT_VALUE]] : $NSArray):
121+
// CHECK: [[CAST_TMP:%.*]] = alloc_stack $Optional<Array<String>>
122+
// CHECK: retain_value [[INPUT_VALUE]]
123+
// CHECK: apply {{%.*}}<Array<String>>([[CAST_TMP]], [[INPUT_VALUE]],
124+
// CHECK: release_value [[INPUT_VALUE]]
125+
// NOTE: In contrast to with take_always, the release_value is above in SUCCESS_BLOCK
126+
// CHECK: switch_enum_addr [[CAST_TMP]] : $*Optional<Array<String>>, case #Optional.none!enumelt: [[FAIL_BB]], default [[SUCCESS_TRAMPOLINE_BB:bb[0-9]+]]
127+
//
128+
// CHECK: [[SUCCESS_TRAMPOLINE_BB]]:
129+
// CHECK: [[PROJ_ENUM:%.*]] = unchecked_take_enum_data_addr [[CAST_TMP]]
130+
// CHECK: copy_addr [take] [[PROJ_ENUM]] to [initialization] [[OUTPUT]]
131+
// CHECK: dealloc_stack [[CAST_TMP]]
132+
// CHECK: br [[SUCCESS_BB]]
133+
// CHECK: } // end sil function 'array_downcast_takeonsuccess'
134+
sil @array_downcast_takeonsuccess : $@convention(thin) (@guaranteed NSArray) -> () {
135+
bb0(%0 : $NSArray):
136+
%4 = alloc_stack $NSArray
137+
retain_value %0 : $NSArray
138+
store %0 to %4 : $*NSArray
139+
%7 = alloc_stack $Array<String>
140+
checked_cast_addr_br take_on_success NSArray in %4 : $*NSArray to Array<String> in %7 : $*Array<String>, bb2, bb3
141+
142+
bb2:
143+
%9 = load %7 : $*Array<String>
144+
%10 = function_ref @guaranteed_swift_array_user : $@convention(thin) <τ_0_0> (@guaranteed Array<τ_0_0>) -> ()
145+
apply %10<String>(%9) : $@convention(thin) <τ_0_0> (@guaranteed Array<τ_0_0>) -> ()
146+
release_value %9 : $Array<String>
147+
dealloc_stack %7 : $*Array<String>
148+
dealloc_stack %4 : $*NSArray
149+
br bb4
150+
151+
bb3:
152+
dealloc_stack %7 : $*Array<String>
153+
destroy_addr %4 : $*NSArray
154+
dealloc_stack %4 : $*NSArray
155+
br bb4
156+
157+
bb4:
158+
%9999 = tuple()
159+
return %9999 : $()
160+
}
161+
162+
// CHECK-LABEL: sil @array_downcast_takealways : $@convention(thin) (@guaranteed NSArray) -> () {
163+
// CHECK: bb0([[ARG:%.*]] : $NSArray):
164+
// CHECK: [[INPUT:%.*]] = alloc_stack $NSArray
165+
// CHECK: retain_value [[ARG]]
166+
// CHECK: store [[ARG]] to [[INPUT]]
167+
// CHECK: [[OUTPUT:%.*]] = alloc_stack $Array<String>
168+
// CHECK: [[INPUT_VALUE:%.*]] = load [[INPUT]]
169+
// CHECK: br [[BRIDGE_BB:bb[0-9]+]]([[INPUT_VALUE]] :
170+
//
171+
// CHECK: [[SUCCESS_BB:bb[0-9]+]]:
172+
// CHECK: [[SUCCESS_VAL:%.*]] = load [[OUTPUT]]
173+
// CHECK: [[CAST_RESULT:%.*]] = apply {{%.*}}<String>([[SUCCESS_VAL]])
174+
// CHECK-NEXT: release_value [[SUCCESS_VAL]]
175+
// CHECK-NEXT: dealloc_stack [[OUTPUT]]
176+
// CHECK-NEXT: dealloc_stack [[INPUT]]
177+
// CHECK-NEXT: br [[EXIT_BB:bb[0-9]+]]
178+
//
179+
// CHECK: [[FAIL_BB:bb[0-9]+]]:
180+
// CHECK-NEXT: dealloc_stack [[CAST_TMP:%.*]]
181+
// CHECK-NEXT: br [[FAIL_NEXT:bb[0-9]+]]
182+
//
183+
// CHECK: [[FAIL_NEXT]]:
184+
// CHECK-NEXT: dealloc_stack [[OUTPUT]]
185+
// CHECK-NEXT: dealloc_stack [[INPUT]]
186+
// CHECK-NEXT: br [[EXIT_BB]]
187+
//
188+
// CHECK: [[EXIT_BB]]:
189+
// CHECK: return
190+
//
191+
// CHECK: [[BRIDGE_BB]]([[INPUT_VALUE:%.*]] : $NSArray):
192+
// CHECK: [[CAST_TMP:%.*]] = alloc_stack $Optional<Array<String>>
193+
// CHECK: retain_value [[INPUT_VALUE]]
194+
// CHECK: apply {{%.*}}<Array<String>>([[CAST_TMP]], [[INPUT_VALUE]],
195+
// CHECK: release_value [[INPUT_VALUE]]
196+
// NOTE: When we perform take_always, this is the take of the cast.
197+
// CHECK: release_value [[INPUT_VALUE]]
198+
// CHECK: switch_enum_addr [[CAST_TMP]] : $*Optional<Array<String>>, case #Optional.none!enumelt: [[FAIL_BB]], default [[SUCCESS_TRAMPOLINE_BB:bb[0-9]+]]
199+
//
200+
// CHECK: [[SUCCESS_TRAMPOLINE_BB]]:
201+
// CHECK: [[PROJ_ENUM:%.*]] = unchecked_take_enum_data_addr [[CAST_TMP]]
202+
// CHECK: copy_addr [take] [[PROJ_ENUM]] to [initialization] [[OUTPUT]]
203+
// CHECK: dealloc_stack [[CAST_TMP]]
204+
// CHECK: br [[SUCCESS_BB]]
205+
// CHECK: } // end sil function 'array_downcast_takealways'
206+
sil @array_downcast_takealways : $@convention(thin) (@guaranteed NSArray) -> () {
207+
bb0(%0 : $NSArray):
208+
%4 = alloc_stack $NSArray
209+
retain_value %0 : $NSArray
210+
store %0 to %4 : $*NSArray
211+
%7 = alloc_stack $Array<String>
212+
checked_cast_addr_br take_always NSArray in %4 : $*NSArray to Array<String> in %7 : $*Array<String>, bb2, bb3
213+
214+
bb2:
215+
%9 = load %7 : $*Array<String>
216+
%10 = function_ref @guaranteed_swift_array_user : $@convention(thin) <τ_0_0> (@guaranteed Array<τ_0_0>) -> ()
217+
apply %10<String>(%9) : $@convention(thin) <τ_0_0> (@guaranteed Array<τ_0_0>) -> ()
218+
release_value %9 : $Array<String>
219+
dealloc_stack %7 : $*Array<String>
220+
dealloc_stack %4 : $*NSArray
221+
br bb4
222+
223+
bb3:
224+
dealloc_stack %7 : $*Array<String>
225+
dealloc_stack %4 : $*NSArray
226+
br bb4
227+
228+
bb4:
229+
%9999 = tuple()
230+
return %9999 : $()
231+
}

0 commit comments

Comments
 (0)