SE-0138: Add UnsafeRawBufferPointer and UnsafeMutableRawBufferPointer… #4969

atrick · 2016-09-23T18:36:48Z

https://github.com/apple/swift-evolution/blob/master/proposals/0138-unsaferawbufferpointer.md

Unsafe[Mutable]RawBufferPointer is a non-owning view over a region of memory as
a Collection of bytes independent of the type of values held in that
memory. Each 8-bit byte in memory is viewed as a UInt8 value.

Reads and writes on memory via Unsafe[Mutable]RawBufferPointer are untyped
operations. Accessing this Collection's bytes does not bind the
underlying memory to UInt8. The underlying memory must be bound
to some trivial type whenever it is accessed via a typed operation.

In addition to the Collection interface, the following methods from
Unsafe[Mutable]RawPointer's interface to raw memory are
provided with debug mode bounds checks: load(fromByteOffset:as:),
storeBytes(of:toByteOffset:as:), and copyBytes(from:count:).

This is only a view into memory and does not own the memory. Copying a value of
type UnsafeMutableRawBufferPointer does not copy the underlying
memory. Assigning an Unsafe[Mutable]RawBufferPointer into a value-based
collection, such as [UInt8] copies bytes out of memory. Assigning into a
subscript range of UnsafeMutableRawBufferPointer copies into memory.

atrick · 2016-09-23T18:37:05Z

@dabrahams Please review for Swift 3.

atrick · 2016-09-23T18:37:22Z

@swift-ci test OS X.

dabrahams · 2016-09-23T20:20:33Z

stdlib/public/core/Arrays.swift.gyb

+
+extension ${Self} {
+  /// Calls a closure with a view of the array's underlying bytes of memory as a
+  /// Collection of `UInt8`.


Doc comment summaries are single sentence fragment.

dabrahams · 2016-09-23T20:25:16Z

stdlib/public/core/Arrays.swift.gyb

+    _ body: (inout UnsafeMutableRawBufferPointer) throws -> R
+  ) rethrows -> R {
+    return try self.withUnsafeMutableBufferPointer {
+      var bytes = UnsafeMutableRawBufferPointer($0)


make this look like the one below

Oh... this is actually really interesting.

The closure argument was 'inout' in order to allow the closure body to invoke a subscript-by-range setter. This was a bit of nonsense inherited from UnsafeMutableBufferPointer, which requires the subscript setter to be mutating. I think it should be nonmutating (because a pointer has reference semantics):

public subscript(bounds: Range<Int>) -> UnsafeMutableRawBufferPointer { nonmutating set { ... } }

This way withUnsafeBytes() does not need to take an inout argument. This is an easy fix for raw buffer pointer. We may want to later revisit whether typed buffer pointer's subscript-by-range should also be nonmutating.

I think I agree with you about the nonmutating thing. Hopefully we can implement a mutating requirement with a nonmutating function...

You ought to be able to. If you can't, that's a bug.

dabrahams · 2016-09-23T20:26:09Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+%  Mutable = 'Mutable' if mutable else ''
+
+/// A non-owning view over a region of memory as a Collection of bytes
+/// independent of the type of values held in that memory. Each 8-bit byte in


Summary: single fragment

dabrahams · 2016-09-23T20:29:23Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+  }
+
+  /// The number of bytes in the buffer.
+  public var count: Int {


I think you don't need this.

Like UnsafeBufferPointer, var endIndex is defined in terms of count. I think the implementations should be consistent so they have the same performance behavior.

Well, you have to define one in terms of the other I guess. The difference is that count has a default implementation so if we made them both store a pair of pointers instead of a pointer + length we'd get it with less code.

dabrahams · 2016-09-23T20:34:37Z

test/stdlib/UnsafeRawBufferPointer.swift

+      }
+    }
+    // Mutable view of array2's bytes.
+    array2.withUnsafeMutableBytes { bytes2 in


The implementation of this tricky and it doesn't look like you're testing the tricky bits: ensuring uniqueness, and preventing undefined behavior if the user violates inout rules.

I've been working on more satisfactory testing, and noticed some issues with UnsafeBufferPointer testing in the process. Once those changes are in master I'll update this PR.

dabrahams · 2016-09-23T20:37:41Z

test/stdlib/UnsafeRawBufferPointer.swift

+
+// Verify basic Sequence/Iterator functionality.
+UnsafeRawBufferPointerTestSuite.test("Sequence") {
+  var array1: [Int32] = [0, 1, 2, 3]


Use prepackaged test components from StdlibUnittest to check for RandomAccessCollection

#4954) https://github.com/apple/swift-evolution/blob/master/proposals/0138-unsaferawbufferpointer.md Unsafe[Mutable]RawBufferPointer is a non-owning view over a region of memory as a Collection of bytes independent of the type of values held in that memory. Each 8-bit byte in memory is viewed as a `UInt8` value. Reads and writes on memory via `Unsafe[Mutable]RawBufferPointer` are untyped operations. Accessing this Collection's bytes does not bind the underlying memory to `UInt8`. The underlying memory must be bound to some trivial type whenever it is accessed via a typed operation. In addition to the `Collection` interface, the following methods from `Unsafe[Mutable]RawPointer`'s interface to raw memory are provided with debug mode bounds checks: `load(fromByteOffset:as:)`, `storeBytes(of:toByteOffset:as:)`, and `copyBytes(from:count:)`. This is only a view into memory and does not own the memory. Copying a value of type `UnsafeMutableRawBufferPointer` does not copy the underlying memory. Assigning an `Unsafe[Mutable]RawBufferPointer` into a value-based collection, such as `[UInt8]` copies bytes out of memory. Assigning into a subscript range of UnsafeMutableRawBufferPointer copies into memory.

Fix some badly written tests with false positives. Generalize the testing to cover raw buffer pointers.

The withUnsafeMutableBytes closure argument should not be `inout`. Improve testing, fix comments. Addresses DaveA's review.

There are several checks related to accessing a slice of an UnsafeBufferPointer. Which tests are active depend on the level of optimization. A raw buffer's checks are also stricter in some cases. This test was originally designed to either crash or not for each input range without regard to the nuances of when bounds checks are enabled. When an input range was marked as crashing, that forced the test case to crash which was self-fullfilling--nothing was really being tested in that case. In my previous checkin, I enabled crash checking to be effective but missed some of the nuances of different bounds checking modes. This commit adds logic to the test to account for these nuances.

atrick · 2016-09-27T18:35:39Z

@dabrahams The inout closure argument qualifier is gone. I'm now testing (hopefully) all of the requirements directly. I'd like to make the claim that this is good enough for 3.0. Let me know if you have any concerns.

atrick · 2016-09-27T18:35:50Z

@swift-ci please test.

atrick · 2016-09-27T18:51:05Z

––– CCC Information –––
• Explanation: SE-0107: UnsafeRawPointer requires users to migrate code using UnsafePointers to a new, strict memory model. The underlying raw pointer was introduced to make this possible, however, the higher-level "buffer" API was not introduced at that time. Experience migrating real applications showed that this functionality is important to help users correctly migrate to 3.0.
• Scope of Issue: Developers migrating to 3.0 with code for binary file formats and streaming I/O.
• Origination: SE-0107 UnsafeRawPointer--missing stdlib functionality required for easy migration to 3.0.
• Risk: Additive API. Code built with 3.0.x might not build with 3.0. The risk of deferring this addition is much higher.
• Reviewed By: Dave Abrahams
• Testing: test/stdlib/UnsafeRawBufferPointer.swift,
validation-test/stdlib/UnsafeBufferPointer.swift.gyb
• Directions for QE: None

atrick · 2016-09-28T21:09:00Z

DaveA signed off on this for 3.0.

atrick · 2016-09-28T21:09:38Z

@swift-ci smoke test OS X platform.

najacque · 2016-09-28T21:35:54Z

Swift OS X platform test passed yesterday: https://ci.swift.org/view/Pull%20Request/job/swift-PR-osx/3845/

tkremenek · 2016-09-29T00:41:27Z

@swift-ci test

dabrahams reviewed Sep 23, 2016

View reviewed changes

atrick added 5 commits September 27, 2016 10:42

Generalize UnsafeBufferPointer-as-a-Collection testing.

ad4a592

Fix some badly written tests with false positives. Generalize the testing to cover raw buffer pointers.

SE-0138: UnsafeRawBufferPointer revision.

069a609

The withUnsafeMutableBytes closure argument should not be `inout`. Improve testing, fix comments. Addresses DaveA's review.

Fix UnsafeBufferPointer validation test: no expectNil on 3.0.

d989323

tkremenek self-assigned this Sep 29, 2016

tkremenek merged commit 303b277 into swiftlang:swift-3.0-branch Sep 29, 2016

SE-0138: Add UnsafeRawBufferPointer and UnsafeMutableRawBufferPointer… #4969

SE-0138: Add UnsafeRawBufferPointer and UnsafeMutableRawBufferPointer… #4969

Uh oh!

Conversation

atrick commented Sep 23, 2016

Uh oh!

atrick commented Sep 23, 2016

Uh oh!

atrick commented Sep 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atrick commented Sep 27, 2016

Uh oh!

atrick commented Sep 27, 2016

Uh oh!

atrick commented Sep 27, 2016

Uh oh!

atrick commented Sep 28, 2016

Uh oh!

atrick commented Sep 28, 2016

Uh oh!

najacque commented Sep 28, 2016

Uh oh!

tkremenek commented Sep 29, 2016

Uh oh!

Uh oh!