Skip to content

Commit 895f136

Browse files
lorenteylorentey
authored
Merge pull request #26760 from lorentey/♪-hold-me-use-me-autorelease-me-♬
[stdlib] AutoreleasingUnsafeMutablePointer: eliminate questionable pointer use
2 parents 40c9e26 + d5ea371 commit 895f136

File tree

4 files changed

+235
-31
lines changed

4 files changed

+235
-31
lines changed

‎stdlib/private/StdlibUnittest/StdlibUnittest.swift

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,14 @@ public func expectFailure(
154154
_anyExpectFailed.orAndFetch(startAnyExpectFailed)
155155
}
156156

157+
/// An opaque function that ignores its argument and returns nothing.
158+
public func noop<T>(_ value: T) {}
159+
160+
/// An opaque function that simply returns its argument.
161+
public func identity<T>(_ value: T) -> T {
162+
return value
163+
}
164+
157165
public func identity(_ element: OpaqueValue<Int>) -> OpaqueValue<Int> {
158166
return element
159167
}

‎stdlib/public/core/BridgeObjectiveC.swift

Lines changed: 39 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,10 @@ public func _getBridgedNonVerbatimObjectiveCType<T>(_: T.Type) -> Any.Type?
336336

337337
// -- Pointer argument bridging
338338

339-
/// A mutable pointer-to-ObjC-pointer argument.
339+
/// A mutable pointer addressing an Objective-C reference that doesn't own its
340+
/// target.
341+
///
342+
/// `Pointee` must be a class type or `Optional<C>` where `C` is a class.
340343
///
341344
/// This type has implicit conversions to allow passing any of the following
342345
/// to a C or ObjC API:
@@ -368,42 +371,50 @@ public struct AutoreleasingUnsafeMutablePointer<Pointee /* TODO : class */>
368371
self._rawValue = _rawValue
369372
}
370373

371-
/// Access the `Pointee` instance referenced by `self`.
374+
/// Retrieve or set the `Pointee` instance referenced by `self`.
375+
///
376+
/// `AutoreleasingUnsafeMutablePointer` is assumed to reference a value with
377+
/// `__autoreleasing` ownership semantics, like `NSFoo **` declarations in
378+
/// ARC. Setting the pointee autoreleases the new value before trivially
379+
/// storing it in the referenced memory.
372380
///
373381
/// - Precondition: the pointee has been initialized with an instance of type
374382
/// `Pointee`.
375383
@inlinable
376384
public var pointee: Pointee {
377-
/// Retrieve the value the pointer points to.
378385
@_transparent get {
379-
// We can do a strong load normally.
380-
return UnsafePointer(self).pointee
386+
// The memory addressed by this pointer contains a non-owning reference,
387+
// therefore we *must not* point an `UnsafePointer<AnyObject>` to
388+
// it---otherwise we would allow the compiler to assume it has a +1
389+
// refcount, enabling some optimizations that wouldn't be valid.
390+
//
391+
// Instead, we need to load the pointee as a +0 unmanaged reference. For
392+
// an extra twist, `Pointee` is allowed (but not required) to be an
393+
// optional type, so we actually need to load it as an optional, and
394+
// explicitly handle the nil case.
395+
let unmanaged =
396+
UnsafePointer<Optional<Unmanaged<AnyObject>>>(_rawValue).pointee
397+
return _unsafeReferenceCast(
398+
unmanaged?.takeUnretainedValue(),
399+
to: Pointee.self)
381400
}
382-
/// Set the value the pointer points to, copying over the previous value.
383-
///
384-
/// AutoreleasingUnsafeMutablePointers are assumed to reference a
385-
/// value with __autoreleasing ownership semantics, like 'NSFoo**'
386-
/// in ARC. This autoreleases the argument before trivially
387-
/// storing it to the referenced memory.
401+
388402
@_transparent nonmutating set {
389403
// Autorelease the object reference.
390-
typealias OptionalAnyObject = AnyObject?
391-
let newAnyObject = unsafeBitCast(newValue, to: OptionalAnyObject.self)
392-
Builtin.retain(newAnyObject)
393-
Builtin.autorelease(newAnyObject)
394-
// Trivially assign it as an OpaquePointer; the pointer references an
395-
// autoreleasing slot, so retains/releases of the original value are
396-
// unneeded.
397-
typealias OptionalUnmanaged = Unmanaged<AnyObject>?
398-
UnsafeMutablePointer<Pointee>(_rawValue).withMemoryRebound(
399-
to: OptionalUnmanaged.self, capacity: 1) {
400-
if let newAnyObject = newAnyObject {
401-
$0.pointee = Unmanaged.passUnretained(newAnyObject)
402-
}
403-
else {
404-
$0.pointee = nil
405-
}
404+
let object = _unsafeReferenceCast(newValue, to: Optional<AnyObject>.self)
405+
Builtin.retain(object)
406+
Builtin.autorelease(object)
407+
408+
// Convert it to an unmanaged reference and trivially assign it to the
409+
// memory addressed by this pointer.
410+
let unmanaged: Optional<Unmanaged<AnyObject>>
411+
if let object = object {
412+
unmanaged = Unmanaged.passUnretained(object)
413+
} else {
414+
unmanaged = nil
406415
}
416+
UnsafeMutablePointer<Optional<Unmanaged<AnyObject>>>(_rawValue).pointee =
417+
unmanaged
407418
}
408419
}
409420

@@ -415,8 +426,7 @@ public struct AutoreleasingUnsafeMutablePointer<Pointee /* TODO : class */>
415426
public subscript(i: Int) -> Pointee {
416427
@_transparent
417428
get {
418-
// We can do a strong load normally.
419-
return (UnsafePointer<Pointee>(self) + i).pointee
429+
return self.advanced(by: i).pointee
420430
}
421431
}
422432

‎stdlib/public/core/Unmanaged.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ public struct Unmanaged<Instance: AnyObject> {
8989
/// and you know that you're not responsible for releasing the result.
9090
///
9191
/// - Returns: The object referenced by this `Unmanaged` instance.
92-
@inlinable // unsafe-performance
92+
@_transparent // unsafe-performance
9393
public func takeUnretainedValue() -> Instance {
9494
return _value
9595
}
@@ -101,7 +101,7 @@ public struct Unmanaged<Instance: AnyObject> {
101101
/// and you know that you're responsible for releasing the result.
102102
///
103103
/// - Returns: The object referenced by this `Unmanaged` instance.
104-
@inlinable // unsafe-performance
104+
@_transparent // unsafe-performance
105105
public func takeRetainedValue() -> Instance {
106106
let result = _value
107107
release()

‎test/stdlib/AutoreleasingUnsafeMutablePointer.swift

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
// RUN: %target-run-simple-swift
2+
// REQUIRES: executable_test
3+
// REQUIRES: objc_interop
4+
5+
import StdlibUnittest
6+
import ObjectiveC // for autoreleasepool
7+
8+
var suite = TestSuite("AutoreleasingUnsafeMutablePointer")
9+
10+
/// Call `body` passing it an AutoreleasingUnsafeMutablePointer whose pointee
11+
/// has the specified value, allocated in a temporary buffer.
12+
func withAUMP<Pointee: AnyObject>(
13+
as type: Pointee.Type = Pointee.self,
14+
initialValue: Optional<Unmanaged<Pointee>> = nil,
15+
_ body: (AutoreleasingUnsafeMutablePointer<Optional<Pointee>>) -> Void
16+
) {
17+
let p =
18+
UnsafeMutablePointer<Optional<Unmanaged<Pointee>>>.allocate(capacity: 1)
19+
p.initialize(to: initialValue)
20+
body(AutoreleasingUnsafeMutablePointer(p))
21+
p.deallocate()
22+
}
23+
24+
/// Call `body` passing it an AutoreleasingUnsafeMutablePointer whose pointee
25+
/// has the specified value, allocated in a temporary buffer.
26+
func withAUMP<Pointee: AnyObject>(
27+
initialValues: [Unmanaged<Pointee>?],
28+
_ body: (AutoreleasingUnsafeMutablePointer<Optional<Pointee>>) -> Void
29+
) {
30+
let p = UnsafeMutablePointer<Optional<Unmanaged<Pointee>>>.allocate(
31+
capacity: initialValues.count
32+
)
33+
for i in 0 ..< initialValues.count {
34+
(p + i).initialize(to: initialValues[i])
35+
}
36+
37+
let aump = AutoreleasingUnsafeMutablePointer<Optional<Pointee>>(p)
38+
body(aump)
39+
p.deallocate()
40+
}
41+
42+
suite.test("helper calls its closure exactly once") {
43+
// `withAUMP` is expected to call its closure exactly once, with an argument
44+
// pointing to a nil value.
45+
var runCount = 0
46+
withAUMP(as: LifetimeTracked.self) { p in
47+
runCount += 1
48+
expectNil(p.pointee)
49+
}
50+
expectEqual(runCount, 1)
51+
}
52+
53+
suite.test("init doesn't autorelease") {
54+
let originalInstances = LifetimeTracked.instances
55+
let unmanaged = Unmanaged.passRetained(LifetimeTracked(42))
56+
expectEqual(LifetimeTracked.instances, originalInstances + 1)
57+
58+
withAUMP(initialValue: unmanaged) { p in noop(p) }
59+
60+
// Releasing the original reference should immediately deallocate the
61+
// object.
62+
expectEqual(LifetimeTracked.instances, originalInstances + 1)
63+
unmanaged.release()
64+
expectEqual(LifetimeTracked.instances, originalInstances)
65+
}
66+
67+
suite.test("getter initially returns the initial value") {
68+
withAUMP(as: LifetimeTracked.self) { p in
69+
expectNil(p.pointee)
70+
}
71+
72+
let unmanaged = Unmanaged.passRetained(LifetimeTracked(42))
73+
withAUMP(initialValue: unmanaged) { p in
74+
expectTrue(p.pointee === unmanaged.takeUnretainedValue())
75+
}
76+
unmanaged.release()
77+
}
78+
79+
suite.test("pointee getter returns the last value set") {
80+
autoreleasepool {
81+
withAUMP(as: LifetimeTracked.self) { p in
82+
expectNil(p.pointee)
83+
let object = LifetimeTracked(23)
84+
p.pointee = object
85+
expectTrue(p.pointee === object)
86+
let other = LifetimeTracked(42)
87+
p.pointee = other
88+
expectTrue(p.pointee === other)
89+
p.pointee = nil
90+
expectNil(p.pointee)
91+
}
92+
}
93+
}
94+
95+
suite.test("pointee getter doesn't autorelease") {
96+
let originalInstances = LifetimeTracked.instances
97+
autoreleasepool {
98+
let unmanaged = Unmanaged.passRetained(LifetimeTracked(42))
99+
expectEqual(LifetimeTracked.instances, originalInstances + 1)
100+
101+
withAUMP(initialValue: unmanaged) { p in
102+
expectTrue(p.pointee === unmanaged.takeUnretainedValue())
103+
}
104+
// Releasing the original reference should immediately deallocate the
105+
// object.
106+
expectEqual(LifetimeTracked.instances, originalInstances + 1)
107+
unmanaged.release()
108+
expectEqual(LifetimeTracked.instances, originalInstances)
109+
}
110+
}
111+
112+
suite.test("pointee setter autoreleases the new value") {
113+
let originalInstances = LifetimeTracked.instances
114+
autoreleasepool {
115+
withAUMP(as: LifetimeTracked.self) { p in
116+
let object = LifetimeTracked(42)
117+
// There is one more instance now.
118+
expectEqual(LifetimeTracked.instances, originalInstances + 1)
119+
p.pointee = object
120+
}
121+
// The strong reference to `object` is gone, but the autoreleasepool
122+
// is still holding onto it.
123+
expectEqual(LifetimeTracked.instances, originalInstances + 1)
124+
}
125+
// Draining the pool deallocates the instance.
126+
expectEqual(LifetimeTracked.instances, originalInstances)
127+
}
128+
129+
suite.test("AutoreleasingUnsafeMutablePointer doesn't retain its value") {
130+
let originalInstances = LifetimeTracked.instances
131+
withAUMP(as: LifetimeTracked.self) { p in
132+
autoreleasepool {
133+
let object = LifetimeTracked(42)
134+
p.pointee = object
135+
expectEqual(LifetimeTracked.instances, originalInstances + 1)
136+
}
137+
// Draining the pool deallocates the instance, even though
138+
// the autoreleasing pointer still points to it.
139+
// This is okay as long as the pointer isn't dereferenced.
140+
expectEqual(LifetimeTracked.instances, originalInstances)
141+
}
142+
expectEqual(LifetimeTracked.instances, originalInstances)
143+
}
144+
145+
suite.test("subscript[0] is the same as pointee.getter") {
146+
withAUMP(as: LifetimeTracked.self) { p in
147+
expectNil(p[0])
148+
}
149+
150+
let unmanaged = Unmanaged.passRetained(LifetimeTracked(42))
151+
withAUMP(initialValue: unmanaged) { p in
152+
expectTrue(p[0] === unmanaged.takeUnretainedValue())
153+
}
154+
unmanaged.release()
155+
}
156+
157+
158+
suite.test("subscript and advanced(by:) works") {
159+
let originalInstances = LifetimeTracked.instances
160+
let count = 100
161+
162+
var refs = 0
163+
let values: [Unmanaged<LifetimeTracked>?] = (0 ..< count).map { i in
164+
if i % 10 == 0 {
165+
refs += 1
166+
return Unmanaged.passRetained(LifetimeTracked(i))
167+
} else {
168+
return nil
169+
}
170+
}
171+
172+
withAUMP(initialValues: values) { p in
173+
for i in 0 ..< count {
174+
expectTrue(p[i] === values[i]?.takeUnretainedValue())
175+
expectTrue(p.advanced(by: i).pointee === values[i]?.takeUnretainedValue())
176+
}
177+
}
178+
179+
expectEqual(LifetimeTracked.instances, originalInstances + refs)
180+
for unmanaged in values {
181+
unmanaged?.release()
182+
}
183+
expectEqual(LifetimeTracked.instances, originalInstances)
184+
}
185+
186+
runAllTests()

0 commit comments

Comments
 (0)