Oh how I detest using pointers in Swift

What did I do wrong?

I think the unwieldy naming is definitely a problem. I would like to see us eventually have unsafe { ... } blocks like Rust and D do, so the Unsafe bit can move out of the names of individual unsafe things. Aside from the redundancy, a naming convention isn't a particularly robust unsafe containment mechanism, because there's nothing keeping you from wrapping up unsafe APIs in other unsafe APIs that don't carry on the naming convention.

The ergonomics of the APIs could be improved as well, and I think we're aware of several areas we could make things better. The focus thus far has been establishing the basic memory model for pointers, not so much on making them pleasant to use yet.

9 Likes

Ok fixed up @John_McCall's suggestion:

// Transform data.
        vertexData.withUnsafeMutableBytes { outputBuffer in
          let outputArray = outputBuffer.bindMemory(to: MCVertex.self)
          vdata.withUnsafeBytes { inputBuffer in
            for i in 0..<mesh.vertexCount {
              let v = inputBuffer.load(fromByteOffset: i*stride+attr.offset, as: float3.self)
              let p = v * scale + translation
              outputArray[i].position.0 = p.x
              outputArray[i].position.1 = p.y
              outputArray[i].position.2 = p.z
            }
          }
        }

Pretty hilarious how "unable to infer complex closure return type" actually means "you forgot a }"

Oh never mind!

Fatal error: load from misaligned raw pointer

This is pretty funny.

Ah, right, loads will definitely assume higher alignment, sorry. Your C code happens to be written in a way that avoids that.

Quite odd that the type checker continues, even when the parser couldn't even match the bracket.

2 Likes

No worries. This is all packed/interleaved vertex data coming from ModelIO, so I don't think there are any alignment guarantees. You just get an offset and stride for the position attribute.

Making raw pointer loads/stores not require alignment for trivial types is AIUI one of the things @Andrew_Trick hopes to fix in the near term.

2 Likes

Ok, so can someone do better than my original ugly code?

Seems like my point is being proved here.

Well, yes and no. This is also proving that your C code may have undefined behavior, which Swift's (verbose, unwieldy) model is protecting you from. In particular, the memcpy in your C code reads four floats each iteration, but only the first three are part of the vertexData position. This means that it may read off the end of the buffer on the final iteration unless vertexData is laid out in memory with explicit padding to allow this.

1 Like

Well I undermined my point by not posting my actual Swift code. The line should be:

_ = vdata.copyBytes(to: ptr, from: start..<start+12)

so I had already fixed that.

But figured "oh that won't look good if I post a hard-coded value" so I actually introduced a bug by changing the 12 to MemoryLayout<float3>.size (16). Oops!

And guess what? Swift didn't save me.

Anyway, point still being proven.

I'll completely set aside the issue of using withXXX closure-taking functions to express variable scope; that's an interesting issue on its own that should be a separate thread.

Naming: Swift pointer names are objectively hilarious. While I do not personally buy into Swift's naming philosophy (I think names are for fast recogonition and disambiguation, while descriptions should be in descriptions), I do appreciate that Swift has a consistent philosophy and there are reasons for it. At least I was able to negotiate to withUnsafeMutableBytes down from withUnsafeMutableRawBufferPointer. I would have preferred the type name to be UnsafeMutableBytes, but I can live without it. The more important design goal here is that Swift programmers rarely need to actually type or even read those names. Given the default safety of Swift, the "Unsafe" prefix does need to be there. The proper solution for that is:

  • use your own type aliases in places where you don't want to be reminded of the unsafety

Documentation: The main issue with Swift programmers hit when working with pointers is that they're trying cast pointer types when they should really avoid that. I want to have:

  • a basic Swift pointer programming guide that steers programmers away from casting pointers and toward proper use of the UnsafeRawPointer API.

  • much better compiler diagnostics with educational notes that point programmers directly to that guide, particularly when they unnecessarily use a "memory binding" API.

Type-casting pointers: Whether you're doing this in C or Swift, there are some serious pitfalls that most programmers are blissfully unaware of because their code will run fine, until one day when it doesn't. Swift's goal is absolutely to stop people from doing this. So, yeah it's painful, but not nearly as painful as it should be.

Memory Binding APIs: If you need to use bindMemory or assumingMemoryBound to work with regular memory that just holds one type, then that's a major usability bug. I think fixing those issues should have a fairly high priority. Here's a list of what can be fixed immediately, roughly in decreasing priority:

  • [SR-10246] Support limited implicit pointer conversion when calling C functions.
  • [SR-4649] Don't require & to pass value to UnsafeRawPointer
  • [SR-10273] Add an UnsafeRaw[Buffer]Pointer API for loading and storing unaligned/packed data
  • [SR-11082] Support withMemoryRebound(to:) with mismatched sizes
  • [SR-11147] Add withUnsafeElements(to: variable) where variable is any homogenous aggregate
  • [SR-11156] Add UnsafePointer[KeyPath] API that applies the field's offset
  • [SR-11087] Add a closure taking API: UnsafeRaw[Mutable][Buffer]Pointer.withMemoryRebound(to:[capacity:])
  • [SR-11300] Provide a RawPointer version of ManagedBuffer
  • API for "load from contiguous as: T" (no bug yet)
  • Allow comparison of raw and typed pointers (no bug filed)
  • Pointers to uninhabited types, OpaquePointer (no bug?)

The basic problem with your example is that Foundation.Data has erased all your element types. That's a higher-level problem with the APIs that I can't speak to. But without fixing that, we could still make straightforward additive changes to the compiler, standard library, and Foundation.Data, so the code could be written like this:

  // Transform vertexData
  vdata.withMemoryRebound(to: float3.self) { inputBuffer in
    vertexData.withMemoryRebound(to: MCVertex.self) { outputBuffer in
      for i in 0..<mesh.vertexCount {
        let v = inputBuffer[i + attr.offset]
        outputBuffer[i].position = v * scale + translation
      }
    }
  }

All we would need for that is:

  • [SR-11087] Add a closure taking API: UnsafeRaw[Mutable][Buffer]Pointer.withMemoryRebound(to:[capacity:])
  • Add an equvalent API do Data.Foundation

Of course, that only makes sense if the data is laid out correctly for the SIMD type. This is the exactly the sort of problem that Swift guards against by forcing you to explicitly rebind memory if you want to use typed pointers.

As you mentioned later in the thread, you're actually reading an unaligned bytestream. So in your case, it makes no sense to use a typed pointer for the input stream. You should explicitly load the values out of a pointer to raw bytes. Unfortunately, that means using copyBytes, until this small API addition is implemented:

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

Then you can do something like what @John_McCall's posted.

16 Likes

Thanks for your thoughtful reply @Andrew_Trick.

Seems like this makes things slightly more pleasant:

extension Data {
    
    func read<T>(to: inout T, offset: Int) {
        Swift.withUnsafeMutableBytes(of: &to) { ptr in
            _ = self.copyBytes(to: ptr, from: offset..<offset+MemoryLayout<T>.size)
        }
    }
    
}

so then I can do just:

        // Transform data.
        vertexData.withUnsafeMutableBytes { buf in
            
            let ptr = buf.baseAddress!.assumingMemoryBound(to: MCVertex.self)
            
            for i in 0..<mesh.vertexCount {
                
                var v = MCVertex() // MCVertex is from a C header for compatibility with Metal.
                let start = i*stride+attr.offset
                vdata.read(to: &v, offset: start)
                
                let p = float3(v.position.0, v.position.1, v.position.2) * scale + translation
                
                ptr[i].position.0 = p.x
                ptr[i].position.1 = p.y
                ptr[i].position.2 = p.z
            }
            
        }

Even better:

extension Data {
    
    func get<T>(value: inout T, offset: Int) {
        Swift.withUnsafeMutableBytes(of: &value) { ptr in
            _ = self.copyBytes(to: ptr, from: offset..<offset+MemoryLayout<T>.size)
        }
    }
    
    mutating func set<T>(value: T, index: Int) {
        
        self.withUnsafeMutableBytes { buf in
            let ptr = buf.baseAddress!.assumingMemoryBound(to: T.self)
            ptr[index] = value
        }
    }
    
}

which allows:

        // Transform data.
        for i in 0..<mesh.vertexCount {
                
            var v = MCVertex()
            let start = i*stride+attr.offset
            vdata.get(value: &v, offset: start)
            
            let p = float3(v.position.0, v.position.1, v.position.2) * scale + translation
            
            vertexData.set(value: MCVertex(position: (p.x, p.y, p.z)), index: i)
        }

Your Data.read extension works. If we actually wanted to add that to Foundation, it would need an "unsafe" prefix ;) Hiding unsafe APIs within safe-looking APIs is generally a concern.

2 Likes

Perhaps one thing to keep in mind is if you make things too painful, developers will just flee to ObjC(++) for the unsafe code. And then what was the point? Most of my app's core data structures, and geometry processing code is written in C++ in part for that reason (and doubts about Swift performance).

Perhaps one thing to keep in mind is if you make things too painful, developers will just flee to ObjC(++) for the unsafe code. And then what was the point? Most of my app's core data structures, and geometry processing code is written in C++ in part for that reason (and doubts about Swift performance).

I misspoke. I don't want to inflict pain. I want to provide direct feedback when code patterns can't be proven safe by the type system or other diagnostics. The necessary complement is that there needs to be a painless way to do the same thing safely. Type-casting pointers is just inherently dangerous and difficult to check. But I recognize it's hard to avoid when passing data between arbitrary C libraries.

2 Likes

I don't think that's the core of the issue. You only use assumingMemoryRebound when converting untyped memory into a typed one. And you'd rarely use bindMemory yourself.

Though there are a few conversion pain point when more and more APIs are using BufferPointers but only non-buffered version gets implicit bridging, including Data APIs.

You forgot about OpaquePointer and Unmanaged<T>. :)

1 Like

Right. There's an impedance mismatch between new Swift code and existing libraries where we always want Swift APIs to use UnsafeBufferPointers but we're importing standalone UnsafePointers. That adds to the name explosion and difficulty building the right pointer type. Over time, I hope we get more Swift APIs on top of libraries that at least use UnsafeBufferPointers, if not safe types.

1 Like
Terms of Service

Privacy Policy

Cookie Policy