Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SR-10273] Add an UnsafeRaw[Buffer]Pointer API for loading and storing unaligned/packed data #52673

Closed
atrick opened this issue Apr 2, 2019 · 9 comments

Comments

@atrick
Copy link
Member

atrick commented Apr 2, 2019

Previous ID SR-10273
Radar rdar://63919502
Original Reporter @atrick
Type New Feature
Status Resolved
Resolution Done
Additional Detail from JIRA
Votes 11
Component/s Compiler, Standard Library
Labels New Feature, LanguageFeatureRequest
Assignee @glessard
Priority Medium

md5: d398d32b35be5db5843911e8ca5112f6

Issue Description:

Swift does not currently provide any reasonable way to load potentially misaligned data from a raw byte buffer.

This API currently requires an aligned pointer:

struct UnsafeRawPointer {
 public func load<T>(fromByteOffset offset: Int = 0, as type: T.Type) -> T {
   _debugPrecondition(0 == (UInt(bitPattern: self + offset)
                      & (UInt(MemoryLayout<T>.alignment) - 1)),
                      "load from misaligned raw pointer")
 //...
}

Users need to be able to do something like this:

receivedData.withUnsafeBytes { (rawPointer) -> Void in
 var offset = 0
 let parsedLength = rawPointer.load(as: UInt8.self)
 offset += MemoryLayout.size(ofValue: parsedLength)
 let parsedType = rawPointer.load(fromByteOffset: offset, as: UInt16.self)
 offset += MemoryLayout.size(ofValue: parsedType)
 ... 
 // Do something with the parsed values
}

We should either remove the alignment restriction from the `UnsafeRawPointer.load` API, or add a new flag to allow unalgined loads. The current default requires alignment because:

  • ignoring alignment may indicate a programmer error, this way you're forced to think about alignment and make it explicit

  • we wanted higher-level APIs to be expressible in terms of raw pointers without changing semantics or losing performance

  • we can loosen the requirement over time but can't strengthen it

@atrick
Copy link
Member Author

atrick commented Apr 2, 2019

Misaligned loads should only be allowed for POD types.

For example, we could reimplement the existing `load` and `storeBytes` using an unaligned Builtin, then broaden the existing assert so that it only performs the alignment check for non-POD types.

@atrick
Copy link
Member Author

atrick commented Apr 2, 2019

It would not be difficult to implement this feature but it does require basic knowledge of all the layers of the compiler. Roughly, I think we need something like:

Define a new AST Builtin for unaligned loads.

In SILGen, add an unaligned flag to `emitBuiltinLoadOrTake`

Add an unaligned flag to PointerToAddressInst in SILInstruction.h

Add all the serialization/deserialization support for that flag.

Change IRGen/GenType.cpp to check for this flag here:

Address TypeInfo::getAddressForPointer(llvm::Value *ptr) const {
 assert(ptr->getType()->getPointerElementType() == StorageType);
 return Address(ptr, getBestKnownAlignment());
}

@atrick
Copy link
Member Author

atrick commented Apr 2, 2019

Once we have a prototype, we need a Swift Evolution proposal to decide whether to change the default UnsafeRawPointer behavior or simply an a new flag.

@belkadan
Copy link
Contributor

belkadan commented Apr 3, 2019

Does the new builtin offer us any benefits over just using memcpy?

@belkadan
Copy link
Contributor

belkadan commented Apr 3, 2019

(which would make this a Standard-Library-only feature request)

@atrick
Copy link
Member Author

atrick commented Apr 3, 2019

The cleanest thing to do would probably be to either

  • change the behavior of the existing Builtin.loadRaw to generate an aligned `pointer_to_address` in SIL

  • or add a new `unaligned` flag to the existing `Builtin.loadRaw` if we need both behaviors

@swift-ci
Copy link
Collaborator

swift-ci commented Jul 29, 2020

Comment by Rick M (JIRA)

Whatever it is would ideally inline nicely.

@atrick
Copy link
Member Author

atrick commented Sep 25, 2020

When the initial API was written, we weren’t sure how we wanted to allow opt-in/opt-out of alignment. It’s possible to move toward allowing misalignment, but not he other way around, hence the current behavior.,

At this point, it’s obvious to me that misalignment should be the default and we need a separate alignment opt-in for code that wants to be vectorized.

Meanwhile, the current workaround goes something like this:

func loadFromData<T>(data: Data, defaultVal: T) -> T {
 data.withUnsafeBytes { dataBytes in
   var value = defaultVal;
   memcpy(&value, dataBytes.baseAddress!, MemoryLayout<T>.size)
   return value
 }
}

@glessard
Copy link
Contributor

glessard commented Apr 16, 2022

This feature was added as part of SE-0349.

Implementation in #40296 and #41033.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants