Initial port to Windows #6

compnerd · 2020-10-07T15:35:16Z

This adds enough shims and tweaks the types sufficiently to allow
building System on Windows.

compnerd · 2020-10-07T15:35:41Z

CC: @kylemacomber @milseman

milseman

Just some initial comments and questions. Glad to see you working on this!

Sources/System/WindowsPlatformConstants.swift

milseman · 2020-10-07T15:44:35Z

Sources/SystemInternals/Exports.swift

+public var system_errno: CInt {
+  get {
+    var value: CInt = 0
+    // TODO(compnerd) handle the error?


What is there to handle?

_get_errno returns an errno_t. It can fail in case of a PEB/TEB corruption or the parameter being invalid.

milseman · 2020-10-07T15:49:42Z

Sources/SystemInternals/Exports.swift

@@ -36,47 +50,129 @@ public var system_errno: CInt {
 #endif

 public func system_open(_ path: UnsafePointer<CChar>, _ oflag: Int32) -> CInt {
+#if os(Windows)
+  var fh: CInt = -1
+  _ = _sopen_s(&fh, path, oflag, _SH_DENYNO, _S_IREAD | _S_IWRITE)


Is this the rough equivalent of POSIX open? E.g., what is the difference between a Windows file handle and a POSIX file descriptor?

Yes, this is roughly the equivalent of the POSIX open.

I don't think that its valuable to try to enumerate every single difference. What type of differences are you really looking for?

But for a taste, there are a large number of differences between a file handle and a POSIX file. The POSIX file is a user mode only, process specific construct. It cannot be shared across processes. It is a mapping from the kernel side representation (HANDLE) and an application specific view. It is maintained by means of a lookup table in the application's address space by the C library. Additionally, it has a comparatively restricted set of operations available to it.

milseman · 2020-10-07T15:50:45Z

Sources/SystemInternals/Exports.swift

+#if os(Windows)
+public func system_open(
+  _ path: UnsafePointer<CChar>, _ oflag: Int32, _ mode: CInt
+) -> CInt {


I wonder if we should try to unify the interface around trivial type casts

I was considering that myself, but wasn't sure if you would be okay with choosing the minimal type across the interfaces.

If we want to do that, I think that doing a follow up might be the best way forward.

milseman · 2020-10-07T15:53:50Z

Sources/SystemInternals/Exports.swift

+    // TODO(compnerd) map windows error to errno
+    return Int(-1)
+  }
+  return Int(nNumberOfBytesRead)


Could you explain more what is going on here? Does Windows not support pread/pwrite? We will have to decide on a case-by-case basis what should be handled by availability and what should be handled by implementing the functionality ourselves (but this doesn't look too bad). This is definitely worthy of being extracted into a helper function and given a good name (and comments!) since it's non-obvious to a non-Windows contributor what's going on.

Yes, there is no pread or pwrite function that is available in the C library. However, the operation itself is supported, but you need to use the Win32 APIs to do that. Basically, I have to convert the file descriptor to the kernel/win32 representation (HANDLE) and then I can perform the operation via the Win32 API (ReadFile or WriteFile) as I don't see the value in going directly to the syscall interface (NtReadFile or NtWriteFile) as they are more complicated to use (it would directly map to the kernel call ZwReadFile or ZwWriteFile).

jakepetroules · 2020-10-07T19:14:58Z

@compnerd How do you plan to handle file paths in SwiftSystem? You probably know what I'm referring to, but to spell it out for the record: 8-bit file paths on Windows are not UTF-8 like on POSIX (except on Windows 10 and even then only in certain cases). Should SwiftSystem go through the Win32 APIs instead, in order to ensure proper Unicode support?

compnerd · 2020-10-07T21:03:48Z

@jakepetroules thats something that I did bring up with @milseman and the plan is to have a representation which is platform dependent AIUI. So that would enable the paths to handle the proper conversion at the right time.

milseman · 2020-10-07T22:06:26Z

Right, so on nix you'd have an initializer taking a collection of bytes in some platform encoding (e.g. UTF-8) and on Windows you'd have an init taking (presumably) a collection of UInt16s. Both platforms can form paths from strings and both platforms can (with potential encoding error correction) go to a string. The storage representation of FilePath is hidden and opaque (even in the ABI).

Sources/System/Errno.swift

milseman · 2020-10-21T17:00:27Z

Sources/SystemInternals/Exports.swift

+  return _lseek(fd, off, whence)
+#else
+  return lseek(fd, off, whence)
+#endif


To reduce the number of #if/#else inside of these functions, should Windows have a internal let lseek = _lseek, etc?

There is a lseek as well, so I don't think that we should be shadowing that.

milseman · 2020-10-21T17:03:04Z

This LGTM after I merge mocking at #8. I apologize in advance that it will cause you some merge conflicts around the system_* functions. Hopefully we can get most of the #if/#else out of there.

milseman · 2020-10-22T18:15:29Z

@compnerd sorry for the conflict, it's worth moving these sys call wrappers into their own file. The good news is that I was able to structure the mocking-related code such that there is less boilerplate and it all appears at the very top of the function, so your code shouldn't need much modification.

compnerd · 2020-10-22T18:36:16Z

That change introduced more than just conflicts :/ The threading model doesn't work for Windows - there is no pthread_t on Windows. I think that I can rebase the change, and we should merge it in the broken state, I'll redo the threading in a follow up.

milseman · 2020-10-22T18:45:13Z

For Windows, would you just use TlsGet/SetValue? This isn't System API, it's in the internals, so it's just an implementation detail. We will definitely want to expose System API for TLS in the future, but that's further down the road and would need a more comprehensive design.

milseman · 2020-10-22T18:46:20Z

If you can show me the Windows calls you'd like use and an example of how to use them, I can refactor the mocking code to accommodate Windows better.

compnerd · 2020-10-22T18:47:55Z

@milseman, @kylemacomber - this part is rebased, I'll do a follow up for the threading

compnerd · 2020-10-22T19:00:52Z

@milseman - lets discuss the threading in #9.

milseman

This LGTM, but I would like to do some cleanup. If there is something blocked by this, that can happen on a subsequent PR. If not, then it might be cleaner to do it here.

Can Windows just define open, pwrite, lseek, etc? That would keep the system_foo bodies clean and straight forward. It's an unfortunate amount of boilerplate we have to have in System while we wait on compiler features (or we do codegen ourselves), so ideally the bodies would be simple and uniform.
We'd like to unify the types for system_write and system_read interfaces as much as possible. I'm worried It might be too easy to break Windows when modifying the System module in what seems to be trivial changes. We can unify on the wider type.

milseman · 2020-10-22T20:47:49Z

Sources/SystemInternals/Syscalls.swift

    return -1
  case .counted(let e, let count):
    assert(count >= 1)
+#if os(Windows)
+    _ = ucrt._set_errno(e)
+#else


I'm definitely in favor of making a helper function to set errno if it means we can avoid scattering #if/#else throughout function bodies. But that can be a subsequent PR.

compnerd · 2020-10-22T20:58:03Z

I tried to do that, but the you cannot do a typealias locally. I don't want to make it shadow the existing functions with the same name. Do you have another suggestion on how to do that? I suppose I can create sys... variants of the functions and use that.
Why would we unify on the wider type? Doesn't that mean that some inputs become invalid? Shouldn't it be the narrower type?

milseman · 2020-10-22T21:10:48Z

One approach might be to add a WindowsSyscalls.swift that has something like:

#If os(Windows)
internal let lseek = _lseek
internal func open(...) {
  var handle: ...
  _open(&handle, ...)
  ...
  return handle
}
...
#endif

The advantage of this is it eliminates the #if in the bodies of the forwarding methods, which eventually will likely number in the hundreds (and we may end up using code generation for). This disadvantage is needing one more jump-to-definition hop when inspecting the source code. This also might be a good place to put the type widening/narrowing. WDYT?

milseman · 2020-10-22T21:23:41Z

For "widening", what I mean is effectively moving the extending/truncating code from FileDescriptor.read into SystemInternals, so that instead of

// FileOperations.swift
extension FileDescriptor {
  func read(...) {
    Int(system_read(self.rawValue, buffer.baseAddress, numericCast(buffer.count)))
  }
}

We'd have:

// WindowsSyscalls.swift
#if os(Windows)
func read(_ fd: Int32, _ buf: UnsafeMutableRawPointer!, _ nbyte: Int) -> Int {
  Int(_read(fd, buf, numericCast(nbyte)))
}
#endif

being semantically equivalent and removing the cognitive burden from anyone touching other parts of System.

milseman · 2020-10-22T23:59:29Z

I could take a stab at this if you like. I wouldn't be able to test it (or even compile it), but it might be a good proof of concept.

milseman · 2020-10-23T00:08:53Z

Sources/SystemInternals/Syscalls.swift

+  var fh: CInt = -1
+  _ = _sopen_s(&fh, path, oflag, _SH_DENYNO, _S_IREAD | _S_IWRITE)
+  return fh
+}


mode is not used, so why do we care to type it as a CInt? For Windows, would we not have a permissions parameter for FileDescriptor.open?.

There is no mode_t type, and I don't think that you can use an internal typealias on a public func.

Should we make the FilePermissions OptionSet empty on Windows? I'm a little uneasy about ignoring information.

no, there is the concept of file permissions - its just that the permissions are different, there are no group permissions, and there are no execute permissions - only read/write.

Ah, then we should we probably have group/execute unavailable on Windows, and the implementation of open here should do the read/write permission setting.

milseman · 2020-10-23T00:21:44Z

#11 is a draft that shows what I mean, let me know what you think.

compnerd · 2020-10-23T00:55:35Z

Hmm, as long as we can ensure that the function gets flattened, I think that might not be a bad idea. I'll play around with that.

milseman · 2020-10-23T15:12:22Z

Do you mean inlined? We can slap an @inline(__always) on them.

compnerd · 2020-10-23T15:25:13Z

Yeah, an @inline(__always) should do the trick - I was thinking of the clang flatten attribute which basically inlines all the way through a call stack, but the same idea.

milseman · 2020-10-23T16:12:10Z

Alright, I think #11 is ready to review. It adds the basic organization and mocking support. If you want attribution for the code, feel free to steal the commits and use them for this branch :-). Up to you. Let me know if I can help with anything else to move this PR forwards.

milseman · 2020-10-23T20:55:35Z

IIUC, the way to resolve the conflict is to remove all changes to Syscalls.swift, given that we now have WindowsSyscallAdapters.swift

Sources/System/Errno.swift

Sources/SystemInternals/Syscalls.swift

milseman · 2020-10-24T16:59:19Z

Sources/SystemInternals/Syscalls.swift

@@ -96,10 +98,10 @@ public func system_close(_ fd: Int32) -> Int32 {

 // read
 public func system_read(
-  _ fd: Int32, _ buf: UnsafeMutableRawPointer!, _ nbyte: Int
-) -> Int {


I think we want the wider types here. Does Windows have a 64-bit offset API for read for 64-bit systems? For whatever reason, size_t is imported into Swift as Int

milseman · 2020-10-24T17:00:16Z

Sources/SystemInternals/Syscalls.swift

-  _ fd: Int32, _ buf: UnsafeRawPointer!, _ nbyte: Int
-) -> Int {
+  _ fd: Int32, _ buf: UnsafeRawPointer!, _ nbyte: CUnsignedInt
+) -> CInt {


Similar to above.

milseman · 2020-10-24T17:01:33Z

Sources/System/FileOperations.swift

@@ -168,7 +168,7 @@ extension FileDescriptor {
    retryOnInterrupt: Bool = true
  ) throws -> Result<Int, Errno> {
    valueOrErrno(retryOnInterrupt: retryOnInterrupt) {
-      system_read(self.rawValue, buffer.baseAddress, buffer.count)
+      Int(system_read(self.rawValue, buffer.baseAddress, numericCast(buffer.count)))


Ideally, this casting would be done in the body of read/write inside the Windows syscall adapters. Can we avoid this at all?

This adds enough shims and tweaks the types sufficiently to allow building System on Windows.

milseman

LGTM! Thanks for all the cleanup!

milseman reviewed Oct 7, 2020

View reviewed changes

benrimmington reviewed Oct 17, 2020

View reviewed changes

Sources/System/Errno.swift Outdated Show resolved Hide resolved

compnerd force-pushed the windows branch from 6a71a71 to 734f1c2 Compare October 19, 2020 15:45

milseman reviewed Oct 21, 2020

View reviewed changes

compnerd force-pushed the windows branch from 734f1c2 to c99ec73 Compare October 22, 2020 18:47

milseman approved these changes Oct 22, 2020

View reviewed changes

milseman reviewed Oct 23, 2020

View reviewed changes

compnerd force-pushed the windows branch from c99ec73 to 66824e5 Compare October 23, 2020 21:32

benrimmington reviewed Oct 23, 2020

View reviewed changes

Sources/System/Errno.swift Outdated Show resolved Hide resolved

milseman reviewed Oct 24, 2020

View reviewed changes

Initial port to Windows

4ffcc62

This adds enough shims and tweaks the types sufficiently to allow building System on Windows.

compnerd force-pushed the windows branch from 66824e5 to 4ffcc62 Compare November 3, 2020 23:05

milseman approved these changes Nov 4, 2020

View reviewed changes

milseman merged commit 0627d28 into apple:main Nov 4, 2020

irshadpc mentioned this pull request Nov 4, 2020

[172] Issue #172 - November 5, 2020 SwiftWeekly/swiftweekly.github.io#556

Closed

compnerd deleted the windows branch November 4, 2020 18:31

Initial port to Windows #6

Initial port to Windows #6

Conversation

compnerd commented Oct 7, 2020

compnerd commented Oct 7, 2020

milseman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakepetroules commented Oct 7, 2020

compnerd commented Oct 7, 2020

milseman commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milseman commented Oct 21, 2020

milseman commented Oct 22, 2020

compnerd commented Oct 22, 2020

milseman commented Oct 22, 2020

milseman commented Oct 22, 2020

compnerd commented Oct 22, 2020

compnerd commented Oct 22, 2020

milseman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compnerd commented Oct 22, 2020

milseman commented Oct 22, 2020 • edited Loading

milseman commented Oct 22, 2020

milseman commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milseman commented Oct 23, 2020

compnerd commented Oct 23, 2020

milseman commented Oct 23, 2020

compnerd commented Oct 23, 2020

milseman commented Oct 23, 2020

milseman commented Oct 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milseman left a comment

Choose a reason for hiding this comment

milseman commented Oct 22, 2020 •

edited

Loading