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

libstd: win: Remove calls to SetHandleInformation #59813

Closed

Conversation

chouquette
Copy link
Contributor

This function isn't available when targeting UWP.
Instead:

  • Create all sockets with the WSA_FLAG_NO_HANDLE_INHERIT flag
    The socket created by accept() need not to explicitely disable handle
    inheritance, since the documentation states «The newly created socket
    is the socket that will handle the actual connection; it has the same
    properties as socket s, including the asynchronous events registered
    with the WSAAsyncSelect or WSAEventSelect functions.»
    The WSA_FLAG_NO_HANDLE_INHERIT is available starting from Windows 7
    (sp1) which matches Rust's target platform for Windows
  • Pass a boolean flag to allow the 'their' end of pipes to be
    inheritable directly upon creating it, instead of a later call to
    SetHandleInformation

As you'd have guessed, this is the first MR of a serie that intend to allow rust to (cross) compile UWP apps. More will come, but I still have to remove quite a few forbidden functions usage.

As a side note, I'm extremely new to rust, so I might have missed something rather obvious, please bare with me :)

This function isn't available when targeting UWP.
Instead:
- Create all sockets with the WSA_FLAG_NO_HANDLE_INHERIT flag
  The socket created by accept() need not to explicitely disable handle
  inheritance, since the documentation states «The newly created socket
  is the socket that will handle the actual connection; it has the same
  properties as socket s, including the asynchronous events registered
  with the WSAAsyncSelect or WSAEventSelect functions.»
  The WSA_FLAG_NO_HANDLE_INHERIT is available starting from Windows 7
  (sp1) which matches Rust's target platform for Windows
- Pass a boolean flag to allow the 'their' end of pipes to be
  inheritable directly upon creating it, instead of a later call to
  SetHandleInformation
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @bluss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2019
@retep998
Copy link
Member

retep998 commented Apr 9, 2019

The WSA_FLAG_NO_HANDLE_INHERIT is available starting from Windows 7
(sp1) which matches Rust's target platform for Windows

While the rust compiler will only run on Windows 7 or newer, it is capable of targeting Windows XP. Making sockets no longer work for XP or Vista would be a breaking change for those users. See #26658 .

Also on the topic of UWP, wouldn't it be better to have it as a separate target entirely? There's going to be a lot of changes that require minimum versions even higher than Windows 7, such as switching from CreateFile to CreateFile2 which will require Windows 8, to say nothing of the hassle with LoadLibrary! Nevermind the fact that different libraries will have to be linked to.

There are some hard decisions to resolve here.

@retep998 retep998 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 9, 2019
@chouquette
Copy link
Contributor Author

Hi,

Ok my bad, I misunderstood the targets. Then I don't think there's a way of removing those SetHandleInformation calls other than conditional compilation.

Regarding your other points, I added a new target locally (i686-pc-uwp-gnu). It's essentially the same as the regular windows (gnu) target, but targets libwindowsapp, libucrt and libvcruntime140_app
When building with llvm-mingw, this automatically enables/disables a bunch of other libraries (as in, the compiler can infer that it's targeting a non desktop target)
The different libraries part is actually fairly easy IMO, especially if building with llvm. When building with gcc, as far as I know it requires dumping the compiler specfile and modifying it.
There are also changes to mingw-w64 that are required (https://sourceforge.net/p/mingw-w64/mailman/mingw-w64-public/?viewmonth=201904)

The adaptation of other libs is indeed going to be time consuming. Some calls might need to be implemented as dummy placeholders in libwinstorecompat, when there's no way of replacing them, or when there are too many calls to forbidden functions that can't be fixed/replaced without cluttering the code too much (or in various dependencies)

But since we're starting to discussing UWP in general, would it be better if I started to commit to a WIP merge request instead as I go?

@retep998
Copy link
Member

It would probably be better to have one big PR that adds new UWP targets and adds a bunch of conditional stuff to libstd to get UWP working in one go.

@Dylan-DPC-zz
Copy link

ping from triage can anyone from @rust-lang/libs review this?

@chouquette
Copy link
Contributor Author

Sorry I should have closed this MR. I'll open a new one with all changes regarding UWP sometime next week, including modification to this patch and another similar one, in order to keep compatibility with Windows XP

@chouquette chouquette closed this Apr 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants