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

Add platform support and syscall mapping docs #110

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

milseman
Copy link
Contributor

No description provided.

@milseman
Copy link
Contributor Author

@lorentey @simonjbeaumont @compnerd I've started to document the details of our support across platforms. I think a good convention would be that API additions, especially syscalls, would update this document.

@milseman
Copy link
Contributor Author

@swift-ci please test

| API | Darwin | Linux | Windows |
|:-------------------|:-----------|:-----------|:-----------|
| `description` | `strerror` | `strerror` | `strerror` |
| `debugDescription` | `strerror` | `strerror` | `strerror` |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to cover the current state or the desired state? I think that for Windows, we should consider either _strerror_s or _wstrerror_s. The downside is that it requires a duplicated call, one to size the buffer which the user allocates and the call to convert the string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This covers the current state, where we will make a String copy from the pointer given to us. This is not the most performance-critical path. What's the advantage of those calls?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage of the calls is avoiding the annoying warnings about deprecation. Microsoft has long been trying to push for users to migrate to APIs that are user-allocated buffers rather than static buffers and to calls that take buffer pointer and size rather than buffer pointer only. These are supposed to be more "secure" as a result (at least should help alleviate some of the buffer overruns).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it doesn't seem as critical in that case since we immediately copy the contents into a new String and do not expose the static buffer pointer. Please open an issue if this is something we should still track.

Documentation/PlatformSupport.md Outdated Show resolved Hide resolved
| `write(fromAbsoluteOffset:)` | `pwrite` | `pwrite` | *custom* |
| `duplicate` | `dup` | `dup` | `_dup` |
| `duplicate(as:)` | `dup2` | `dup2` | `_dup2` |
| `resize` | `ftruncate` | `ftruncate` | *N/A* |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ftruncate is possible to implement, though it requires a custom implementation. I think that it was likely just missed when the API was added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really sorry, but I have trouble understanding why this table needs to exist.

I beg y'all, please let's not pretend that Windows provides a syscall interface that matches Darwin or Linux. This package really isn't supposed to be platform-independent -- each platform ought to have its own set of APIs, tailored to its own peculiarities. Let each platform shine. Let each platform have its own native interfaces. Don't force one platform to needlessly (and badly) emulate the pointless minutiae of another! Embrace & expose the differences, as faithfully as possible.

Creating a set of portable APIs is a very desirable and extremely important goal; however, this package most certainly cannot fulfill that goal without losing its original reason for existence -- which was to enable native system programming without the complexities of C APIs naively imported into Swift.

The question shouldn't be whether it's possible to implement something on Windows that looks kind of like ftruncate from a distance -- rather, the question ought to be what can we do to faithfully expose the real _wsopen_s interface, or to make calling CreateFileA/W a pleasant experience in Swift.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this set of interfaces are meant to be nearly identical (there are some differences in the sense that some require an extra parameter or such). Windows does have a POSIX (to the letter) subsystem, so these interfaces are present. Of course, being to the letter, optional interfaces may not present. resize is not part of the POSIX standard that it implements, but the functionality to truncate the file is more or less just spelling differences.


| API | Darwin | Linux | Windows |
|:-------|:-------|:-------|:--------|
| `pipe` | `pipe` | `pipe` | *N/A* |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sadly, my requests that the Windows side be added at the same time were ignored :-(.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Windows SDK is generally different enough that I think it makes sense for it to have its own code base, mostly/entirely separate from the rest of this package. I don't think one can convincingly argue that CreatePipe is close enough to pipe(2) that they need to have precisely the same API in swift-system, only differing on the backend -- can we?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants