Skip to content

Commit 26719c6

Browse files
swift-ciswift-ci
authored
Merge pull request swiftlang#9198 from devincoughlin/access-enforcement-closure-capture
2 parents 9354a93 + de9c646 commit 26719c6

File tree

4 files changed

+164
-21
lines changed

4 files changed

+164
-21
lines changed

‎lib/SILOptimizer/Mandatory/AccessEnforcementSelection.cpp

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,10 @@ class SelectEnforcement {
8585
void analyzeUsesOfBox(SILInstruction *source);
8686
void analyzeProjection(ProjectBoxInst *projection);
8787

88+
/// Note that the given instruction is a use of the box (or a use of
89+
/// a projection from it) in which the address escapes.
90+
void noteEscapingUse(SILInstruction *inst);
91+
8892
void propagateEscapes();
8993
void propagateEscapesFrom(SILBasicBlock *bb);
9094

@@ -137,20 +141,7 @@ void SelectEnforcement::analyzeUsesOfBox(SILInstruction *source) {
137141
continue;
138142

139143
// Treat everything else as an escape:
140-
141-
// Add it to the escapes set.
142-
Escapes.insert(user);
143-
144-
// Record this point as escaping.
145-
auto userBB = user->getParent();
146-
auto &state = StateMap[userBB];
147-
if (!state.IsInWorklist) {
148-
state.HasEscape = true;
149-
state.IsInWorklist = true;
150-
Worklist.push_back(userBB);
151-
}
152-
assert(state.HasEscape);
153-
assert(state.IsInWorklist);
144+
noteEscapingUse(user);
154145
}
155146

156147
assert(!Accesses.empty() && "didn't find original access!");
@@ -165,9 +156,34 @@ void SelectEnforcement::analyzeProjection(ProjectBoxInst *projection) {
165156
if (access->getEnforcement() == SILAccessEnforcement::Unknown)
166157
Accesses.push_back(access);
167158
}
159+
160+
// FIXME: We should distinguish between escapes via arguments to escaping
161+
// closures (which must propagate to to all reachable blocks because they
162+
// could access the address at any later point in the program) and
163+
// non-escaping closures (which only force dynamic enforcement for
164+
// accesses that are in progress at the time of the escape).
165+
if (auto partialApply = dyn_cast<PartialApplyInst>(user)) {
166+
noteEscapingUse(partialApply);
167+
}
168168
}
169169
}
170170

171+
void SelectEnforcement::noteEscapingUse(SILInstruction *inst) {
172+
// Add it to the escapes set.
173+
Escapes.insert(inst);
174+
175+
// Record this point as escaping.
176+
auto userBB = inst->getParent();
177+
auto &state = StateMap[userBB];
178+
if (!state.IsInWorklist) {
179+
state.HasEscape = true;
180+
state.IsInWorklist = true;
181+
Worklist.push_back(userBB);
182+
}
183+
assert(state.HasEscape);
184+
assert(state.IsInWorklist);
185+
}
186+
171187
void SelectEnforcement::propagateEscapes() {
172188
while (!Worklist.empty()) {
173189
auto bb = Worklist.pop_back_val();

‎test/Interpreter/enforce_exclusive_access.swift

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,19 +101,60 @@ ExclusiveAccessTestSuite.test("ModifyFollowedByModify") {
101101
globalX = X() // no-trap
102102
}
103103

104-
ExclusiveAccessTestSuite.test("ClosureCaptureModifyModify") {
104+
ExclusiveAccessTestSuite.test("ClosureCaptureModifyModify")
105+
.skip(.custom(
106+
{ _isFastAssertConfiguration() },
107+
reason: "this trap is not guaranteed to happen in -Ounchecked"))
108+
.crashOutputMatches("modify/modify access conflict detected on address")
109+
.code
110+
{
111+
var x = X()
112+
modifyAndPerform(&x) {
113+
expectCrashLater()
114+
x.i = 12
115+
}
116+
}
117+
118+
ExclusiveAccessTestSuite.test("ClosureCaptureReadModify")
119+
.skip(.custom(
120+
{ _isFastAssertConfiguration() },
121+
reason: "this trap is not guaranteed to happen in -Ounchecked"))
122+
.crashOutputMatches("read/modify access conflict detected on address")
123+
.code
124+
{
105125
var x = X()
106126
modifyAndPerform(&x) {
107-
// FIXME: This should be caught dynamically.
127+
expectCrashLater()
128+
_blackHole(x.i)
129+
}
130+
}
131+
132+
ExclusiveAccessTestSuite.test("ClosureCaptureModifyRead")
133+
.skip(.custom(
134+
{ _isFastAssertConfiguration() },
135+
reason: "this trap is not guaranteed to happen in -Ounchecked"))
136+
.crashOutputMatches("modify/read access conflict detected on address")
137+
.code
138+
{
139+
var x = X()
140+
readAndPerform(&x) {
141+
expectCrashLater()
108142
x.i = 12
109143
}
110144
}
111145

146+
ExclusiveAccessTestSuite.test("ClosureCaptureReadRead") {
147+
var x = X()
148+
readAndPerform(&x) {
149+
_blackHole(x.i) // no-trap
150+
}
151+
}
152+
112153
// Test for per-thread enforcement. Don't trap when two different threads
113154
// have overlapping accesses
114155
ExclusiveAccessTestSuite.test("PerThreadEnforcement") {
115156
modifyAndPerform(&globalX) {
116-
var (_, otherThread) = _stdlib_pthread_create_block(nil, { (_ : Void) -> () in
157+
let (_, otherThread) = _stdlib_pthread_create_block(nil, { (_ : Void) -> () in
117158
globalX.i = 12 // no-trap
118159
return ()
119160
}, ())

‎test/SILOptimizer/access_enforcement_selection.sil

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,41 @@ sil hidden @markFuncEscape : $@convention(thin) () -> () {
6464
%99 = return %98 : $()
6565
}
6666

67+
68+
sil @takesInoutAndClosure : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
69+
sil @closureCapturingByStorageAddress : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
70+
71+
// Test dynamic enforcement of box addresses that escape via closure
72+
// partial_applys.
73+
// application.
74+
// CHECK-LABEL: sil hidden @escapeAsArgumentToPartialApply : $@convention(thin) () -> () {
75+
// CHECK: bb0:
76+
// CHECK: [[BOX:%.*]] = alloc_box ${ var Builtin.Int64 }, var, name "x"
77+
// CHECK: [[ADR:%.*]] = project_box [[BOX]] : ${ var Builtin.Int64 }, 0
78+
// CHECK: [[FUNC:%.*]] = function_ref @takesInoutAndClosure : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
79+
// CHECK: [[CLOSURE:%.*]] = function_ref @closureCapturingByStorageAddress : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
80+
// CHECK: [[PA:%.*]] = partial_apply [[CLOSURE]]([[ADR]]) : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
81+
// CHECK: [[MODIFY:%.*]] = begin_access [modify] [dynamic] [[ADR]] : $*Builtin.Int64
82+
// CHECK: %{{.*}} = apply [[FUNC]]([[MODIFY]], [[PA]]) : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
83+
// CHECK: end_access [[MODIFY]] : $*Builtin.Int64
84+
// CHECK: destroy_value [[BOX]] : ${ var Builtin.Int64 }
85+
// CHECK: return %{{.*}} : $()
86+
// CHECK-LABEL:} // end sil function 'escapeAsArgumentToPartialApply'
87+
sil hidden @escapeAsArgumentToPartialApply : $@convention(thin) () -> () {
88+
%2 = alloc_box ${ var Builtin.Int64 }, var, name "x"
89+
%3 = project_box %2 : ${ var Builtin.Int64 }, 0
90+
%4 = function_ref @takesInoutAndClosure : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
91+
%5 = function_ref @closureCapturingByStorageAddress : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
92+
%6 = partial_apply %5(%3) : $@convention(thin) (@inout_aliasable Builtin.Int64) -> ()
93+
%7 = begin_access [modify] [unknown] %3 : $*Builtin.Int64
94+
%8 = apply %4(%7, %6) : $@convention(thin) (@inout Builtin.Int64, @owned @callee_owned () -> ()) -> ()
95+
end_access %7 : $*Builtin.Int64
96+
destroy_value %2 : ${ var Builtin.Int64 }
97+
%9 = tuple ()
98+
%10 = return %9 : $()
99+
}
100+
101+
67102
// Test static enforcement of copied boxes.
68103
// FIXME: Oops... We make this dynamic.
69104
//

‎test/SILOptimizer/access_enforcement_selection.swift

Lines changed: 55 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@ public func takesInout(_ i: inout Int) {
1212
// Helper taking a basic, no-escape closure.
1313
func takeClosure(_: ()->Int) {}
1414

15+
// Helper taking a basic, no-escape closure.
16+
func takeClosureAndInout(_: inout Int, _: ()->Int) {}
17+
18+
// Helper taking an escaping closure.
19+
func takeEscapingClosure(_: @escaping ()->Int) {}
1520

1621
// Generate an alloc_stack that escapes into a closure.
1722
public func captureStack() -> Int {
@@ -21,8 +26,10 @@ public func captureStack() -> Int {
2126
return x
2227
}
2328
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection12captureStackSiyF
24-
// Static access for `return x`.
25-
// CHECK: Static Access: %{{.*}} = begin_access [read] [static] %{{.*}} : $*Int
29+
// Dynamic access for `return x`. Since the closure is non-escaping, using
30+
// dynamic enforcement here is more conservative than it needs to be -- static
31+
// is sufficient here.
32+
// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
2633

2734
// The access inside the closure is dynamic, until we have the logic necessary to
2835
// prove that no other closures are passed to `takeClosure` that may write to
@@ -31,6 +38,49 @@ public func captureStack() -> Int {
3138
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection12captureStackSiyFSiycfU_
3239
// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
3340

41+
42+
// Generate an alloc_stack that does not escape into a closure.
43+
public func nocaptureStack() -> Int {
44+
var x = 3
45+
takeClosure { return 5 }
46+
return x
47+
}
48+
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14nocaptureStackSiyF
49+
// Static access for `return x`.
50+
// CHECK: Static Access: %{{.*}} = begin_access [read] [static] %{{.*}} : $*Int
51+
//
52+
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14nocaptureStackSiyFSiycfU_
53+
54+
// Generate an alloc_stack that escapes into a closure while an access is
55+
// in progress.
56+
public func captureStackWithInoutInProgress() -> Int {
57+
// Use a `var` so `x` isn't treated as a literal.
58+
var x = 3
59+
takeClosureAndInout(&x) { return x }
60+
return x
61+
}
62+
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection31captureStackWithInoutInProgressSiyF
63+
// Dynamic access for `&x`. This must be dynamic so that we catch the conflict
64+
// in the closure.
65+
// CHECK-DAG: Dynamic Access: %{{.*}} = begin_access [modify] [dynamic] %{{.*}} : $*Int
66+
// Dynamic access for `return x`. This is more conservative than it needs
67+
// to be; it can be static.
68+
// CHECK-DAG: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
69+
//
70+
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection31captureStackWithInoutInProgressSiyFSiycfU_
71+
// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
72+
73+
// Generate an alloc_box that escapes into a closure.
74+
public func captureBox() -> Int {
75+
var x = 3
76+
takeEscapingClosure { x = 4; return x }
77+
return x
78+
}
79+
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection10captureBoxSiyF
80+
// Dynamic access for `return x`.
81+
// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
82+
// CHECK-LABEL: _T028access_enforcement_selection10captureBoxSiyFSiycfU_
83+
3484
// Generate a closure in which the @inout_aliasing argument
3585
// escapes to an @inout function `bar`.
3686
public func recaptureStack() -> Int {
@@ -40,8 +90,9 @@ public func recaptureStack() -> Int {
4090
}
4191
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14recaptureStackSiyF
4292
//
43-
// Static access for `return x`.
44-
// CHECK: Static Access: %{{.*}} = begin_access [read] [static] %{{.*}} : $*Int
93+
// Dynamic access for `return x`. This is more conservative than it needs
94+
// to be; static is sufficient.
95+
// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
4596

4697
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14recaptureStackSiyFSiycfU_
4798
//

0 commit comments

Comments
 (0)