Skip to content

Commit fdf69f0

Browse files
committed
Use GetValueAtEndOfBlock instead of GetValueInMiddleOfBlock for getting values in exits.
Otherwise we will leak values if the copy block is in the same block as the terminator and we fail to identify a single destroy for the copy block. We are generally pretty good at identifying those single destroy cases, so I am not sure how often this actually happens. Found by the ownership verifier. Original Commit: 3b0fcf9. <rdar://problem/53376468>
1 parent b65580c commit fdf69f0

File tree

2 files changed

+158
-1
lines changed

2 files changed

+158
-1
lines changed

lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -758,7 +758,7 @@ static bool fixupCopyBlockWithoutEscaping(CopyBlockWithoutEscapingInst *CB,
758758
auto *SafeClosureDestructionPt =
759759
getDeinitSafeClosureDestructionPoint(Term);
760760
SILBuilderWithScope B(SafeClosureDestructionPt);
761-
B.createDestroyValue(generatedLoc, Updater.GetValueInMiddleOfBlock(Exit));
761+
B.createDestroyValue(generatedLoc, Updater.GetValueAtEndOfBlock(Exit));
762762
}
763763
}
764764

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
// RUN: %target-sil-opt -enable-sil-verify-all -closure-lifetime-fixup %s | %FileCheck %s
2+
3+
sil_stage raw
4+
5+
import Swift
6+
import Builtin
7+
import SwiftShims
8+
9+
class FakeNSString {}
10+
class Klass {}
11+
12+
sil @$sSSSgIgg_AAIegg_TR : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
13+
sil @noescapeBlock3 : $@convention(c) (Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<FakeNSString>) -> ()
14+
sil @$sSS10FoundationE19_bridgeToObjectiveCSo8FakeNSStringCyF : $@convention(method) (@guaranteed String) -> @owned FakeNSString
15+
sil @$sSS21_builtinStringLiteral17utf8CodeUnitCount7isASCIISSBp_BwBi1_tcfC : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String
16+
sil @$sSSSgIegg_So8FakeNSStringCSgIyBy_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> ()
17+
18+
// Just make sure that we perform the optimization and do not trigger the ownership verifier.
19+
//
20+
// CHECK-LABEL: sil [ossa] @test1 : $@convention(thin) (@guaranteed Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, @guaranteed Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>) -> () {
21+
// CHECK-NOT: convert_escape_to_noescape [not_guaranteed]
22+
// CHECK: } // end sil function 'test1'
23+
sil [ossa] @test1 : $@convention(thin) (@guaranteed Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, @guaranteed Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>) -> () {
24+
bb0(%0 : @guaranteed $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, %1 : @guaranteed $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>):
25+
%2 = copy_value %0 : $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>
26+
switch_enum %2 : $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb12
27+
28+
bb1(%4 : @owned $@callee_guaranteed (@guaranteed Optional<String>) -> ()):
29+
%5 = convert_escape_to_noescape [not_guaranteed] %4 : $@callee_guaranteed (@guaranteed Optional<String>) -> () to $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
30+
%6 = enum $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, #Optional.some!enumelt.1, %5 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
31+
destroy_value %4 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
32+
br bb2(%6 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>)
33+
34+
bb2(%9 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>):
35+
switch_enum %9 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, case #Optional.some!enumelt.1: bb3, case #Optional.none!enumelt: bb4
36+
37+
bb3(%11 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()):
38+
%12 = function_ref @$sSSSgIgg_AAIegg_TR : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
39+
%13 = partial_apply [callee_guaranteed] %12(%11) : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
40+
%14 = mark_dependence %13 : $@callee_guaranteed (@guaranteed Optional<String>) -> () on %11 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
41+
%15 = copy_value %14 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
42+
%16 = alloc_stack $@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
43+
%17 = project_block_storage %16 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
44+
store %15 to [init] %17 : $*@callee_guaranteed (@guaranteed Optional<String>) -> ()
45+
%19 = function_ref @$sSSSgIegg_So8FakeNSStringCSgIyBy_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> ()
46+
%20 = init_block_storage_header %16 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), invoke %19 : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> (), type $@convention(block) @noescape (Optional<FakeNSString>) -> ()
47+
%21 = copy_block_without_escaping %20 : $@convention(block) @noescape (Optional<FakeNSString>) -> () withoutEscaping %14 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
48+
%22 = enum $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, #Optional.some!enumelt.1, %21 : $@convention(block) @noescape (Optional<FakeNSString>) -> ()
49+
destroy_addr %17 : $*@callee_guaranteed (@guaranteed Optional<String>) -> ()
50+
dealloc_stack %16 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
51+
br bb5(%22 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>)
52+
53+
bb4:
54+
%26 = enum $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, #Optional.none!enumelt
55+
br bb5(%26 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>)
56+
57+
bb5(%28 : @owned $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>):
58+
%29 = copy_value %1 : $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>
59+
switch_enum %29 : $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, case #Optional.some!enumelt.1: bb6, case #Optional.none!enumelt: bb11
60+
61+
bb6(%31 : @owned $@callee_guaranteed (@guaranteed Optional<String>) -> ()):
62+
%32 = convert_escape_to_noescape [not_guaranteed] %31 : $@callee_guaranteed (@guaranteed Optional<String>) -> () to $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
63+
%33 = enum $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, #Optional.some!enumelt.1, %32 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
64+
destroy_value %31 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
65+
br bb7(%33 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>)
66+
67+
bb7(%36 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>):
68+
switch_enum %36 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, case #Optional.some!enumelt.1: bb8, case #Optional.none!enumelt: bb9
69+
70+
bb8(%38 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()):
71+
%39 = function_ref @$sSSSgIgg_AAIegg_TR : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
72+
%40 = partial_apply [callee_guaranteed] %39(%38) : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
73+
%41 = mark_dependence %40 : $@callee_guaranteed (@guaranteed Optional<String>) -> () on %38 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
74+
%42 = copy_value %41 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
75+
%43 = alloc_stack $@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
76+
%44 = project_block_storage %43 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
77+
store %42 to [init] %44 : $*@callee_guaranteed (@guaranteed Optional<String>) -> ()
78+
%46 = function_ref @$sSSSgIegg_So8FakeNSStringCSgIyBy_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> ()
79+
%47 = init_block_storage_header %43 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), invoke %46 : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> (), type $@convention(block) @noescape (Optional<FakeNSString>) -> ()
80+
%48 = copy_block_without_escaping %47 : $@convention(block) @noescape (Optional<FakeNSString>) -> () withoutEscaping %41 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
81+
%49 = enum $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, #Optional.some!enumelt.1, %48 : $@convention(block) @noescape (Optional<FakeNSString>) -> ()
82+
destroy_addr %44 : $*@callee_guaranteed (@guaranteed Optional<String>) -> ()
83+
dealloc_stack %43 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
84+
br bb10(%49 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>)
85+
86+
bb9:
87+
%53 = enum $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, #Optional.none!enumelt
88+
br bb10(%53 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>)
89+
90+
bb10(%55 : @owned $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>):
91+
%56 = string_literal utf8 "Foobar"
92+
%57 = integer_literal $Builtin.Word, 6
93+
%58 = integer_literal $Builtin.Int1, -1
94+
%59 = metatype $@thin String.Type
95+
%60 = function_ref @$sSS21_builtinStringLiteral17utf8CodeUnitCount7isASCIISSBp_BwBi1_tcfC : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String
96+
%61 = apply %60(%56, %57, %58, %59) : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String
97+
%62 = function_ref @$sSS10FoundationE19_bridgeToObjectiveCSo8FakeNSStringCyF : $@convention(method) (@guaranteed String) -> @owned FakeNSString
98+
%63 = begin_borrow %61 : $String
99+
%64 = apply %62(%63) : $@convention(method) (@guaranteed String) -> @owned FakeNSString
100+
end_borrow %63 : $String
101+
%66 = enum $Optional<FakeNSString>, #Optional.some!enumelt.1, %64 : $FakeNSString
102+
destroy_value %61 : $String
103+
%68 = function_ref @noescapeBlock3 : $@convention(c) (Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<FakeNSString>) -> ()
104+
%69 = apply %68(%28, %55, %66) : $@convention(c) (Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<FakeNSString>) -> ()
105+
destroy_value %66 : $Optional<FakeNSString>
106+
destroy_value %55 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>
107+
destroy_value %28 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>
108+
%73 = tuple ()
109+
return %73 : $()
110+
111+
bb11:
112+
%75 = enum $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, #Optional.none!enumelt
113+
br bb7(%75 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>)
114+
115+
bb12:
116+
%77 = enum $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, #Optional.none!enumelt
117+
br bb2(%77 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>)
118+
}
119+
120+
sil @originalClosure : $@convention(thin) () -> ()
121+
sil @noEscapeThunk : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
122+
sil @blockThunk : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed () -> ()) -> ()
123+
124+
// Just make sure we apply the optimization. The ownership verifier will verify
125+
// that we do not catch the leak.
126+
//
127+
// CHECK-LABEL: sil [ossa] @ssaupdater_no_single_destroy_some_in_exit_block : $@convention(thin) (@guaranteed Klass, @guaranteed @callee_guaranteed () -> (), @guaranteed Klass, @guaranteed @callee_guaranteed () -> ()) -> () {
128+
// CHECK-NOT: convert_escape_to_noescape [not_guaranteed]
129+
// CHECK-NOT: copy_block_without_escaping
130+
// CHECK: } // end sil function 'ssaupdater_no_single_destroy_some_in_exit_block'
131+
sil [ossa] @ssaupdater_no_single_destroy_some_in_exit_block : $@convention(thin) (@guaranteed Klass, @guaranteed @callee_guaranteed () -> (), @guaranteed Klass, @guaranteed @callee_guaranteed () -> ()) -> () {
132+
bb0(%0 : @guaranteed $Klass, %1 : @guaranteed $@callee_guaranteed () -> (), %2 : @guaranteed $Klass, %3 : @guaranteed $@callee_guaranteed () -> ()):
133+
// This basic block is needed to trigger the bug.
134+
br bb1
135+
136+
bb1:
137+
%39 = function_ref @originalClosure : $@convention(thin) () -> ()
138+
%43 = partial_apply [callee_guaranteed] %39() : $@convention(thin) () -> ()
139+
%44 = convert_escape_to_noescape [not_guaranteed] %43 : $@callee_guaranteed () -> () to $@noescape @callee_guaranteed () -> ()
140+
%45 = function_ref @noEscapeThunk : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
141+
%46 = partial_apply [callee_guaranteed] %45(%44) : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
142+
%47 = mark_dependence %46 : $@callee_guaranteed () -> () on %44 : $@noescape @callee_guaranteed () -> ()
143+
%48 = copy_value %47 : $@callee_guaranteed () -> ()
144+
%49 = alloc_stack $@block_storage @callee_guaranteed () -> ()
145+
%50 = project_block_storage %49 : $*@block_storage @callee_guaranteed () -> ()
146+
store %48 to [init] %50 : $*@callee_guaranteed () -> ()
147+
%52 = function_ref @blockThunk : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed () -> ()) -> ()
148+
%53 = init_block_storage_header %49 : $*@block_storage @callee_guaranteed () -> (), invoke %52 : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed () -> ()) -> (), type $@convention(block) @noescape () -> ()
149+
%54 = copy_block_without_escaping %53 : $@convention(block) @noescape () -> () withoutEscaping %47 : $@callee_guaranteed () -> ()
150+
%55 = enum $Optional<@convention(block) @noescape () -> ()>, #Optional.some!enumelt.1, %54 : $@convention(block) @noescape () -> ()
151+
destroy_addr %50 : $*@callee_guaranteed () -> ()
152+
dealloc_stack %49 : $*@block_storage @callee_guaranteed () -> ()
153+
destroy_value %43 : $@callee_guaranteed () -> ()
154+
destroy_value %55 : $Optional<@convention(block) @noescape () -> ()>
155+
%86 = tuple ()
156+
return %86 : $()
157+
}

0 commit comments

Comments
 (0)