SE-0333: Expand usability of `withMemoryRebound` #39529

glessard · 2021-09-30T21:31:15Z

Implementation of SE-0333, "Expand usability of withMemoryRebound. See proposal here.

Addresses bugs SR-11082, SR-11087, rdar://88087816

- updated documentation and adjust implementations

glessard · 2022-02-03T18:49:24Z

@swift-ci please test

glessard · 2022-02-03T19:14:15Z

@swift-ci please test

glessard · 2022-02-03T21:08:12Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

  /// - Returns: The return value, if any, of the `body` closure parameter.
  @inlinable // unsafe-performance
+  @_alwaysEmitIntoClient
+% if Mutable:
+  @_silgen_name("$_swift_UnsafeMutableBufferPointer_se0333_withMemoryRebound")


These getting-out-of-the-way silgen names are a new thing in the stdlib, as far as I can tell.
Is this naming scheme reasonable? Too fussy?
This is likely to be the precedent for other similar actions in the future, so agreement is desirable.

I think that these names are fine; I like that they explicitly point to the proposal for reference.

glessard · 2022-02-03T21:10:09Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+% else:
+  @_silgen_name("$sSR17withMemoryRebound2to_qd_0_qd__m_qd_0_SRyqd__GKXEtKr0_lF")
+% end
+  public func _legacy_withMemoryRebound<T, Result>(


Similarly, the public name for a symbol that being pushed out of the way is a new thing.
Is this naming scheme reasonable? This is likely to be the precedent for other similar actions in the future.

I think I might explicitly put _legacy_se0333_withMemoryRebound or similar here, but I also don't really want to bikeshed these, because users should never need to worry about them.

glessard · 2022-02-03T21:13:59Z

Note that the additions to UnsafeRawPointer, UnsafeMutableRawPointer, UnsafeRawBufferPointer and UnsafeMutableRawBufferPointer are @_alwaysEmitIntoClient at the moment. This is debatable; we could decide to make them simply @inlinable, and live with the ABI implications.

glessard · 2022-02-03T21:17:48Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+% else:
+  @_silgen_name("$sSR17withMemoryRebound2to_qd_0_qd__m_qd_0_SRyqd__GKXEtKr0_lF")
+% end
+  public func _legacy_withMemoryRebound<T, Result>(
    to type: T.Type, _ body: (${Self}<T>) throws -> Result
  ) rethrows -> Result {
    if let base = _position {


This preserves the old implementation verbatim, but we could also forward to the new implementation. Forwarding to the new implementation would be better from a testability standpoint, but less rigourously correct from a compatibility standpoint. However, we are lifting a restriction so the new implementation should be compatible (e.g. it passes the old test). Thoughts?

I think I agree we should forward it! The code looks clearly compatible for well-written code.

There is a minor chance of a binary compatibility issue if someone is calling this with mismatching strides; they might rely on the resulting buffer pointer having the same count as the original. But the original debug precondition seems like a big enough obstacle to prevent that for actually occurring in practice.

glessard · 2022-02-03T21:18:29Z

@swift-ci please test

swift-ci · 2022-02-03T23:28:41Z

Build failed
Swift Test OS X Platform
Git Sha - 5c29236a8c88e480ffa87b3b36606ed7164fbcbe

swift-ci · 2022-02-04T00:11:19Z

Build failed
Swift Test Linux Platform
Git Sha - 5c29236a8c88e480ffa87b3b36606ed7164fbcbe

glessard · 2022-02-04T01:32:33Z

@swift-ci please smoke test

glessard · 2022-02-04T03:03:42Z

@swift-ci please smoke test

glessard · 2022-02-04T19:02:00Z

The output of nm -j <path-to>/libswiftCore.dylib is only one line different after applying this series of changes, and that’s _$ss6UInt32V_A63BtMD. I don’t know where that comes from, but it doesn't register with the ABI checker test.

I believe the ABI checker test is in error when it says that

+Func UnsafeBufferPointer.withMemoryRebound(to:_:) has been removed
+Func UnsafeMutableBufferPointer.withMemoryRebound(to:_:) has been removed
+Func UnsafeMutablePointer.withMemoryRebound(to:capacity:_:) has been removed
+Func UnsafePointer.withMemoryRebound(to:capacity:_:) has been removed

lorentey

This looks good to me!

Having nothing else to complain about, I picked a few ridiculously tiny nits; take them or ignore them as you like. 😉

lorentey · 2022-02-04T22:51:00Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+% else:
+  @_silgen_name("$sSR17withMemoryRebound2to_qd_0_qd__m_qd_0_SRyqd__GKXEtKr0_lF")
+% end
+  public func _legacy_withMemoryRebound<T, Result>(
    to type: T.Type, _ body: (${Self}<T>) throws -> Result
  ) rethrows -> Result {
    if let base = _position {


I think I agree we should forward it! The code looks clearly compatible for well-written code.

There is a minor chance of a binary compatibility issue if someone is calling this with mismatching strides; they might rely on the resulting buffer pointer having the same count as the original. But the original debug precondition seems like a big enough obstacle to prevent that for actually occurring in practice.

stdlib/public/core/UnsafePointer.swift

lorentey · 2022-02-04T23:01:26Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+      return try body(.init(start: nil, count: 0))
+    }
+    _debugPrecondition(
+      Int(bitPattern: s) & (MemoryLayout<T>.alignment-1) == 0


(Low prio note) We should probably introduce a utility predicate for this. (s._isAligned(to: T.self), or something like that?)

stdlib/public/core/UnsafeRawPointer.swift

glessard · 2022-02-04T23:27:02Z

@swift-ci please smoke test

glessard · 2022-02-05T04:25:46Z

The test "api-digester/stability-stdlib-abi-with-asserts.test" says:

+Func UnsafeBufferPointer.withMemoryRebound(to:_:) has been removed
+Func UnsafeMutableBufferPointer.withMemoryRebound(to:_:) has been removed
+Func UnsafeMutablePointer.withMemoryRebound(to:capacity:_:) has been removed
+Func UnsafePointer.withMemoryRebound(to:capacity:_:) has been removed

This is an error on the part of the test.
The output of nm -j <path-to>/libswiftCore.dylib is identical before and after this change (after filtering out local symbols.)

glessard · 2022-02-05T05:05:32Z

@swift-ci please smoke test

glessard · 2022-02-05T09:48:23Z

@swift-ci please smoke test

glessard · 2022-02-05T10:13:11Z

@swift-ci please test macOS platform

glessard marked this pull request as draft September 30, 2021 21:31

glessard force-pushed the se-withMemoryRebound branch from 483dcf6 to 712918c Compare October 7, 2021 21:07

glessard force-pushed the se-withMemoryRebound branch 4 times, most recently from f072d4b to 23fa269 Compare November 17, 2021 22:00

glessard force-pushed the se-withMemoryRebound branch 2 times, most recently from 7753049 to cc85b89 Compare December 3, 2021 11:09

[stdlib] update existing withMemoryRebound

c2dfb94

- updated documentation and adjust implementations

glessard force-pushed the se-withMemoryRebound branch from cc85b89 to b850838 Compare February 3, 2022 01:16

glessard changed the title ~~[DNM] Draft implementation for pitch: Expand usability of withMemoryRebound~~ [DNM] Implement SE-0333: Expand usability of withMemoryRebound Feb 3, 2022

glessard changed the title ~~[DNM] Implement SE-0333: Expand usability of withMemoryRebound~~ [DNM] SE-0333: Expand usability of withMemoryRebound Feb 3, 2022

glessard force-pushed the se-withMemoryRebound branch from b850838 to 133222d Compare February 3, 2022 15:43

glessard marked this pull request as ready for review February 3, 2022 18:49

glessard requested review from lorentey and stephentyrone February 3, 2022 18:49

glessard changed the title ~~[DNM] SE-0333: Expand usability of withMemoryRebound~~ SE-0333: Expand usability of withMemoryRebound Feb 3, 2022

glessard force-pushed the se-withMemoryRebound branch from 16cca8c to 5c29236 Compare February 3, 2022 21:03

glessard commented Feb 3, 2022

View reviewed changes

glessard force-pushed the se-withMemoryRebound branch from 3ed0741 to 78a2558 Compare February 4, 2022 01:32

[abi] preserve original implementations of withMemoryRebound

5f5c75d

glessard added 8 commits February 3, 2022 20:02

[stdlib] add U[M]RP.withMemoryRebound

3de1eac

[stdlib] add U[M]RBP.withMemoryRebound

5403129

[stdlib] add U[M]RBP.assumingMemoryBound

c248326

[stdlib] documentation clarifications

2454264

[gardening] fix warnings in tests

2eb37a8

[test] rebinding typed memory to wider and narrower types

69aa3c3

[test] rebinding raw memory

9f2c4b5

[test] assumingMemoryBound on raw memory buffers

72f4e65

glessard force-pushed the se-withMemoryRebound branch from 78a2558 to 72f4e65 Compare February 4, 2022 03:03

glessard added 2 commits February 4, 2022 15:48

[abi] make the compatibility symbols internal

2a9fa0c

[abi] forward calls made to the old withMemoryRebound symbols

ba416cd

lorentey approved these changes Feb 4, 2022

View reviewed changes

[gardening] formatting adjustments

3f67313

[test] override the judgment of the ABI digester test

b2aaaeb

glessard force-pushed the se-withMemoryRebound branch from 2a2bb17 to b2aaaeb Compare February 5, 2022 09:44

glessard merged commit 1161f92 into swiftlang:main Feb 5, 2022

glessard deleted the se-withMemoryRebound branch February 6, 2022 13:28

This was referenced Feb 7, 2022

[SE-0333] Mark as implemented swiftlang/swift-evolution#1541

Merged

[stdlib] Implement withContiguousStorageIfAvailable for raw buffer types #41288

Merged

finagolfin mentioned this pull request Feb 28, 2022

[stdlib] relax stride check in UnsafePointer.withMemoryRebound #41553

Merged

glessard mentioned this pull request Mar 29, 2022

[stdlib] remove preconditions from compatibility entry point #42076

Merged

This was referenced Feb 4, 2022

[SR-11082] Support withMemoryRebound(to:) with mismatched sizes #53474

Closed

[SR-11087] Add a closure taking API: UnsafeRaw[Mutable][Buffer]Pointer.withMemoryRebound(to:[capacity:]) #53479

Closed

SE-0333: Expand usability of withMemoryRebound #39529

SE-0333: Expand usability of withMemoryRebound #39529

Uh oh!

Conversation

glessard commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glessard commented Feb 3, 2022

Uh oh!

glessard commented Feb 3, 2022

Uh oh!

glessard Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentyrone Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

glessard Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

stephentyrone Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

glessard Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

glessard commented Feb 3, 2022

Uh oh!

glessard Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorentey Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

glessard commented Feb 3, 2022

Uh oh!

swift-ci commented Feb 3, 2022

Uh oh!

swift-ci commented Feb 4, 2022

Uh oh!

glessard commented Feb 4, 2022

Uh oh!

glessard commented Feb 4, 2022

Uh oh!

glessard commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lorentey left a comment

Choose a reason for hiding this comment

Uh oh!

lorentey Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lorentey Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glessard commented Feb 4, 2022

Uh oh!

glessard commented Feb 5, 2022

Uh oh!

glessard commented Feb 5, 2022

Uh oh!

glessard commented Feb 5, 2022

Uh oh!

glessard commented Feb 5, 2022

Uh oh!

Uh oh!

SE-0333: Expand usability of `withMemoryRebound` #39529

SE-0333: Expand usability of `withMemoryRebound` #39529

glessard commented Sep 30, 2021 •

edited

Loading

glessard Feb 3, 2022 •

edited

Loading

glessard Feb 3, 2022 •

edited

Loading

glessard commented Feb 4, 2022 •

edited

Loading