-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Remove delay-binding for Win XP and Vista #81250
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (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. |
Tagging @rylev @joshtriplett |
r? @joshtriplett - I don't think I can review this, but feel free to reassign of course. |
@joshtriplett Ok. Just tagged you because Ryan said that you had been part of the discussions on target platform support / tiers. |
@sivadeilra Mark-simulacrum and joshtriplett are different people :P |
Looks reasonable to me. r=me as soon as @rylev confirms. |
Typo:
should be vista not 7 |
Looks fine to me though I'm not familiar with the APIs in question to give a definitive review. Perhaps @m-ou-se could take a look as she's made several changes in this area lately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
A few small comments:
The minimum supported Windows version is now Windows 7. Windows XP and Windows Vista are no longer supported; both are already broken, and require extra steps to use. This commit removes the delayed-binding support for Windows API functions that are present on all supported Windows targets. This has several benefits: Removes needless complexity. Removes a load and dynamic call on hot paths in mutex acquire / release. This may have performance benefits. * "Drop official support for Windows XP" rust-lang/compiler-team#378 * "Firefox has ended support for Windows XP and Vista" https://support.mozilla.org/en-US/kb/end-support-windows-xp-and-vista
834dec2
to
59855e0
Compare
Thanks, @m-ou-se. I've updated with feedback, merged & rebased, and pushed an update. |
@bors r=joshtriplett,m-ou-se |
📌 Commit 59855e0 has been approved by |
☀️ Test successful - checks-actions |
Tested on commit rust-lang/rust@9a9477f. Direct link to PR: <rust-lang/rust#81250> 💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
I'll take a look at the miri break. |
…ulacrum Remove outdated comment in windows' mutex.rs After rust-lang#81250, this `Mutex` no longer falls back to the `ReentrantMutex` implementation, so this comment is no longer relevant.
I'd like to revert this change if possible. I'm curious if this actually improved performance. I have no practical need for XP but it's now broken. Who are the Tier 3 "target maintainers"? It's a little confusing since "i686-pc-windows-msvc" and "x86_64-pc-windows-msvc" are all listed as tier 1 (Windows 7+) and as tier 3 (32/64-bit Windows XP). I'd also like to know if we should think about this change in context with the "i586-pc-windows-msvc" tier 2 as I don't see a need to build a non-SSE version of the standard library If performance actually did empirically improve I'd be inclined to do what Rust does best here and optimize the choice of API calls at compile time. CPUs are pretty smart these days so I'd feel better if there is test ouptut to back up this change. If reverting isn't possible, I propose introducing an "msvclegacy" triple to replace "i586-pc-windows-msvc", "x86_64-pc-windows-msvc", and "i586-pc-windows-msvc" because those Tier 3 target names are contradicting with Tier 1. These new triples would also be the proper target for even older efforts such as the one by @seritools. I'm happy to create a ticket if a new set of triples is the right path. I'm also happy to revert the docs to show that |
I think this would be the right thing to do. The Rust std does not officially support Windows versions before Windows 7. Even when it did, it was broken in various other ways due to lack of maintainers and testing (which is a big part of why it's no longer supported). Reverting this PR wouldn't be enough to support XP. If there are people willing to commit to maintaining a new XP (or even 9x) target then I'd personally very much welcome them. They would need to follow the new target tier process you linked to or else maintain their own fork. |
For now I also see old windows support as something a fork would be better suited to as there are lots and lots of small and big limitations, depending on how far "down" in Windows support you dare to go. @RandomInsano since I know you're interested, this was my list of notes and known limitations for rust9x back from when I had the time to work on it :^) DetailsNotes
Fallback limitationsA lot of APIs have limitations on older systems - refer to old MSDN Libraries (e.g. from VS 2005)
Synchronization primitives
Default allocator on 9x/MEFrom old MSDN:
It might make sense to use an alternative allocator instead. |
Sounds good. Considering the additional pieces that failed to work before, I think the fork is the right approach for now.
I’ll open that PR for changing the docs now.
… On Mar 1, 2022, at 2:22 PM, Dennis Duda ***@***.***> wrote:
For now I also see old windows support as something a fork would be better suited to as there are lots and lots of small and big limitations, depending on how far "down" in Windows support you dare to go.
@RandomInsano since I know you're interested, this was my list of notes and known limitations for rust9x back from when I had the time to work on it :^)
Details
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you were mentioned.
|
Documentation was missed when demoting Windows XP to no_std only After a quick discussion on rust-lang#81250 which removed special casing for mutexes added [here](rust-lang@10b103a) to support Windows XP, we can't say that the standard library can build for it. This change modifies the tier 3 non-ARM targets to show the standard library will no longer build for these and there is no work being done to change that.
Documentation was missed when demoting Windows XP to no_std only After a quick discussion on rust-lang#81250 which removed special casing for mutexes added [here](rust-lang@10b103a) to support Windows XP, we can't say that the standard library can build for it. This change modifies the tier 3 non-ARM targets to show the standard library will no longer build for these and there is no work being done to change that.
Documentation was missed when demoting Windows XP to no_std only After a quick discussion on rust-lang#81250 which removed special casing for mutexes added [here](rust-lang@10b103a) to support Windows XP, we can't say that the standard library can build for it. This change modifies the tier 3 non-ARM targets to show the standard library will no longer build for these and there is no work being done to change that.
Documentation was missed when demoting Windows XP to no_std only After a quick discussion on rust-lang#81250 which removed special casing for mutexes added [here](rust-lang@10b103a) to support Windows XP, we can't say that the standard library can build for it. This change modifies the tier 3 non-ARM targets to show the standard library will no longer build for these and there is no work being done to change that.
Documentation was missed when demoting Windows XP to no_std only After a quick discussion on rust-lang#81250 which removed special casing for mutexes added [here](rust-lang@10b103a) to support Windows XP, we can't say that the standard library can build for it. This change modifies the tier 3 non-ARM targets to show the standard library will no longer build for these and there is no work being done to change that.
Documentation was missed when demoting Windows XP to no_std only After a quick discussion on rust-lang#81250 which removed special casing for mutexes added [here](rust-lang@10b103a) to support Windows XP, we can't say that the standard library can build for it. This change modifies the tier 3 non-ARM targets to show the standard library will no longer build for these and there is no work being done to change that.
Documentation was missed when demoting Windows XP to no_std only After a quick discussion on rust-lang#81250 which removed special casing for mutexes added [here](rust-lang@10b103a) to support Windows XP, we can't say that the standard library can build for it. This change modifies the tier 3 non-ARM targets to show the standard library will no longer build for these and there is no work being done to change that.
Documentation was missed when demoting Windows XP to no_std only After a quick discussion on rust-lang#81250 which removed special casing for mutexes added [here](rust-lang@10b103a) to support Windows XP, we can't say that the standard library can build for it. This change modifies the tier 3 non-ARM targets to show the standard library will no longer build for these and there is no work being done to change that.
Documentation was missed when demoting Windows XP to no_std only After a quick discussion on rust-lang#81250 which removed special casing for mutexes added [here](rust-lang@10b103a) to support Windows XP, we can't say that the standard library can build for it. This change modifies the tier 3 non-ARM targets to show the standard library will no longer build for these and there is no work being done to change that.
The minimum supported Windows version is now Windows 7. Windows XP
and Windows Vista are no longer supported; both are already broken, and
require extra steps to use.
This commit removes the delayed-binding support for Windows API
functions that are present on all supported Windows targets. This has
several benefits: Removes needless complexity. Removes a load and
dynamic call on hot paths in mutex acquire / release. This may have
performance benefits.
"Drop official support for Windows XP"
Drop official support for Windows XP compiler-team#378
"Firefox has ended support for Windows XP and Vista"
https://support.mozilla.org/en-US/kb/end-support-windows-xp-and-vista