From b1683c89befdda58a8f3181506b58afde2d09e13 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 14 May 2021 18:29:35 +0100 Subject: [PATCH 1/3] Remove as much checked math as possible from buffer pointers. This patch removes as much checked math as I was able to find from the unsafe buffer pointers types. In particular, I removed two kinds of checked math: 1. Checked math that was provably unable to trap due to prior operations done in the same function, or in a function that must have been called before the current one. For example, in index(_:offsetBy:limitedBy:), when we do the final index addition we have already validated that it must be in-bounds. Therefore, it cannot overflow. 2. Checked math that can only fail if the user is misusing the indices. As previously discussed with Karoy and Andrew, the unsafe raw buffer types are not obligated to crash if you misuse their indices, they are free to invoke undefined behaviour. In these cases, I added defensive overflow checks in debug mode. The result of this change should be reductions in code size in almost all usage sites of the raw buffer types. --- .../public/core/UnsafeBufferPointer.swift.gyb | 50 ++++++++++++++++--- .../core/UnsafeRawBufferPointer.swift.gyb | 13 +++-- test/SILOptimizer/unsafebufferpointer.swift | 2 +- 3 files changed, 52 insertions(+), 13 deletions(-) diff --git a/stdlib/public/core/UnsafeBufferPointer.swift.gyb b/stdlib/public/core/UnsafeBufferPointer.swift.gyb index 59372435150a9..22ee90f9ec85a 100644 --- a/stdlib/public/core/UnsafeBufferPointer.swift.gyb +++ b/stdlib/public/core/UnsafeBufferPointer.swift.gyb @@ -143,7 +143,12 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - return i + 1 + // NOTE: Unchecked math because unsafe pointers are not required to + // behave sensibly if you misuse their indices. We validate in + // debug mode. + let result = i.addingReportingOverflow(1) + _debugPrecondition(!result.overflow) + return result.partialValue } @inlinable // unsafe-performance @@ -153,7 +158,12 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - i += 1 + // NOTE: Unchecked math because unsafe pointers are not required to + // behave sensibly if you misuse their indices. We validate in + // debug mode. + let result = i.addingReportingOverflow(1) + _debugPrecondition(!result.overflow) + i = result.partialValue } @inlinable // unsafe-performance @@ -163,7 +173,12 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - return i - 1 + // NOTE: Unchecked math because unsafe pointers are not required to + // behave sensibly if you misuse their indices. We validate in + // debug mode. + let result = i.subtractingReportingOverflow(1) + _debugPrecondition(!result.overflow) + return result.partialValue } @inlinable // unsafe-performance @@ -173,7 +188,12 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - i -= 1 + // NOTE: Unchecked math because unsafe pointers are not required to + // behave sensibly if you misuse their indices. We validate in + // debug mode. + let result = i.subtractingReportingOverflow(1) + _debugPrecondition(!result.overflow) + i = result.partialValue } @inlinable // unsafe-performance @@ -183,7 +203,12 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - return i + n + // NOTE: Unchecked math because unsafe pointers are not required to + // behave sensibly if you misuse their indices. We validate in + // debug mode. + let result = i.addingReportingOverflow(n) + _debugPrecondition(!result.overflow) + return result.partialValue } @inlinable // unsafe-performance @@ -193,11 +218,19 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - let l = limit - i + // NOTE: Unchecked math because unsafe pointers are not required to + // behave sensibly if you misuse their indices. We validate in + // debug mode. + let maxOffset = limit.subtractingReportingOverflow(i) + _debugPrecondition(!maxOffset.overflow) + let l = maxOffset.partialValue + if n > 0 ? l >= 0 && l < n : l <= 0 && n < l { return nil } - return i + n + + // No need to check here, for the same index misuse reasons. + return i &+ n } @inlinable // unsafe-performance @@ -233,7 +266,8 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle @inlinable // unsafe-performance public var indices: Indices { - return startIndex..(start: nil, count: 0) } - let count = (_end! - base) / MemoryLayout.stride + let count = (_end._unsafelyUnwrappedUnchecked - base) / MemoryLayout.stride let typed = base.initializeMemory( as: type, repeating: repeatedValue, count: count) return UnsafeMutableBufferPointer(start: typed, count: count) @@ -644,9 +648,10 @@ extension Unsafe${Mutable}RawBufferPointer { return (it, UnsafeMutableBufferPointer(start: nil, count: 0)) } + _internalInvariant(_end != nil) for p in stride(from: base, // only advance to as far as the last element that will fit - to: base + count - elementStride + 1, + to: _end._unsafelyUnwrappedUnchecked - elementStride + 1, by: elementStride ) { // underflow is permitted -- e.g. a sequence into diff --git a/test/SILOptimizer/unsafebufferpointer.swift b/test/SILOptimizer/unsafebufferpointer.swift index adf1c27328e81..a58db2a63fca1 100644 --- a/test/SILOptimizer/unsafebufferpointer.swift +++ b/test/SILOptimizer/unsafebufferpointer.swift @@ -80,7 +80,7 @@ public func testSubscript(_ ubp: UnsafeBufferPointer) -> Float { // CHECK: [[CMP:%[0-9]+]] = icmp eq // CHECK: br i1 [[CMP]], label %[[RET:.*]], label %[[LOOP]] // -// CHECK: [[RET]]: ; preds = %[[LOOP]], %entry +// CHECK: [[RET]]: ; preds = %[[LOOP]], %{{.*}} // CHECK: ret i64 public func testSubscript(_ ubp: UnsafeRawBufferPointer) -> Int64 { var sum: Int64 = 0 From a3fdcfa43ff37c5847e12c280199c6740a3890d9 Mon Sep 17 00:00:00 2001 From: Karoy Lorentey Date: Thu, 3 Feb 2022 11:39:44 -0800 Subject: [PATCH 2/3] Reword note on wrapping arithmetic --- .../public/core/UnsafeBufferPointer.swift.gyb | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/stdlib/public/core/UnsafeBufferPointer.swift.gyb b/stdlib/public/core/UnsafeBufferPointer.swift.gyb index 22ee90f9ec85a..4f50a6ca9a6fc 100644 --- a/stdlib/public/core/UnsafeBufferPointer.swift.gyb +++ b/stdlib/public/core/UnsafeBufferPointer.swift.gyb @@ -143,9 +143,11 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - // NOTE: Unchecked math because unsafe pointers are not required to - // behave sensibly if you misuse their indices. We validate in - // debug mode. + // NOTE: Wrapping math because we allow unsafe buffer pointers not to verify + // index preconditions in release builds. Our (optimistic) assumption is + // that the caller is already ensuring that indices are valid, so we can + // elide the usual checks to help the optimizer generate better code. + // However, we still check for overflow in debug mode. let result = i.addingReportingOverflow(1) _debugPrecondition(!result.overflow) return result.partialValue @@ -158,9 +160,7 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - // NOTE: Unchecked math because unsafe pointers are not required to - // behave sensibly if you misuse their indices. We validate in - // debug mode. + // See note on wrapping arithmetic in `index(after:)` above. let result = i.addingReportingOverflow(1) _debugPrecondition(!result.overflow) i = result.partialValue @@ -173,9 +173,7 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - // NOTE: Unchecked math because unsafe pointers are not required to - // behave sensibly if you misuse their indices. We validate in - // debug mode. + // See note on wrapping arithmetic in `index(after:)` above. let result = i.subtractingReportingOverflow(1) _debugPrecondition(!result.overflow) return result.partialValue @@ -188,9 +186,7 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - // NOTE: Unchecked math because unsafe pointers are not required to - // behave sensibly if you misuse their indices. We validate in - // debug mode. + // See note on wrapping arithmetic in `index(after:)` above. let result = i.subtractingReportingOverflow(1) _debugPrecondition(!result.overflow) i = result.partialValue @@ -203,9 +199,7 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - // NOTE: Unchecked math because unsafe pointers are not required to - // behave sensibly if you misuse their indices. We validate in - // debug mode. + // See note on wrapping arithmetic in `index(after:)` above. let result = i.addingReportingOverflow(n) _debugPrecondition(!result.overflow) return result.partialValue @@ -218,9 +212,7 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle // optimizer is not capable of creating partial specializations yet. // NOTE: Range checks are not performed here, because it is done later by // the subscript function. - // NOTE: Unchecked math because unsafe pointers are not required to - // behave sensibly if you misuse their indices. We validate in - // debug mode. + // See note on wrapping arithmetic in `index(after:)` above. let maxOffset = limit.subtractingReportingOverflow(i) _debugPrecondition(!maxOffset.overflow) let l = maxOffset.partialValue From 0a82ef8e13e9fef96e5ee90e24e280e78a6ab01c Mon Sep 17 00:00:00 2001 From: Karoy Lorentey Date: Thu, 3 Feb 2022 11:40:41 -0800 Subject: [PATCH 3/3] Re-add debug-mode overflow check in `index(_:,_offsetBy:limit:)` --- stdlib/public/core/UnsafeBufferPointer.swift.gyb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/stdlib/public/core/UnsafeBufferPointer.swift.gyb b/stdlib/public/core/UnsafeBufferPointer.swift.gyb index 4f50a6ca9a6fc..7b4d56f4917cc 100644 --- a/stdlib/public/core/UnsafeBufferPointer.swift.gyb +++ b/stdlib/public/core/UnsafeBufferPointer.swift.gyb @@ -221,8 +221,9 @@ extension Unsafe${Mutable}BufferPointer: ${Mutable}Collection, RandomAccessColle return nil } - // No need to check here, for the same index misuse reasons. - return i &+ n + let result = i.addingReportingOverflow(n) + _debugPrecondition(!result.overflow) + return result.partialValue } @inlinable // unsafe-performance