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 explicit XP targets #66801

Closed
wants to merge 5 commits into from
Closed

Add explicit XP targets #66801

wants to merge 5 commits into from

Conversation

ChrisDenton
Copy link
Member

Windows XP support in Rust can generously be described as anaemic (if not outright broken) and yet the fully supported Windows 7+ target is partially reliant on maintaining some compatibility with XP (e.g. the runtime compatibility layer in libstd). So this PR adds two target triples for XP, allowing 32-bit and 64-bit Windows XP to be targetted separately from Windows 7+. For now the targets are literally a copy/paste of the current i686-pc-windows-msvc and x86_64-pc-windows-msvc, though this could be improved if someone is willing to tackle XP support more thoroughly.

I think this is advantageous whatever the future of XP in Rust. If support is dropped completely then this could potentially provide a more graceful transition before removing it. However if anybody is willing to maintain an XP target then having an explicit triple can only help. And even if XP support remains, as it is now, in a kind of limbo then at least crates will be able to target XP separately from Windows 7+ (although unsurprisingly most crates ignore XP support entirely).

In any case allowing the pc-windows targets to be focused on Windows7+ support would be an advantage.

@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 @varkor (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 Nov 27, 2019
@crlf0710
Copy link
Member

crlf0710 commented Nov 27, 2019

@ChrisDenton Windows XP compatible compilation need to properly specify the /subsystem property to the linker.

Crate config Target Type Linker Flags
normal crate normal msvc target specify nothing or /SUBSYSTEM:console
#![windows_subsystem(x)] crate normal msvc target specify /SUBSYSTEM:x
normal crate xp msvc target 32bit specify /SUBSYSTEM:console,5.01
#![windows_subsystem(x)] crate xp msvc target 32bit specify /SUBSYSTEM:x,5.01
normal crate xp msvc target 64bit specify /SUBSYSTEM:console,5.02
#![windows_subsystem(x)] crate xp msvc target 64bit specify /SUBSYSTEM:x,5.02

Here's the ms document for these:
https://docs.microsoft.com/en-us/cpp/build/reference/subsystem-specify-subsystem?view=vs-2019

Extra notes: MINGW GNU flavor linker syntax is "--subsystem which:major.minor", changing the comma to a semicolon.
https://manpages.debian.org/unstable/binutils-mingw-w64-x86-64/x86_64-w64-mingw32-ld.1.en.html

@ChrisDenton
Copy link
Member Author

Thanks. My thinking was to provide these targets as a baseline to be improved if there is an appetite for it. Windows XP is already considered a "tier 3" platform using pc-windows-msvc as is and I'm not confident enough with Rust's codegen to be sure of my changes to it.

However, I'll attempt the changes. On my first attempt I missed your note about MINGW so I'll need to rethink that commit.

@ChrisDenton
Copy link
Member Author

Hopefully this commit is more robust. There's basically two parts to it. It adds an optional version parameter to the Linker subsystem function. To make use of this it also changes the windows_subsystem field of CodegenResults to be this struct instead of a string:

pub struct WindowsSubsystem {
    pub name: String,
    pub version: Option<String>
}

The part that may be more iffy is where I override Windows XP targets to set the subsystem version.

@varkor
Copy link
Member

varkor commented Nov 27, 2019

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned varkor Nov 27, 2019
@petrochenkov
Copy link
Contributor

What's the problem with the dynamic fallbacks as they exist now?

I now care not even about XP/Vista, but about ReactOS, which may lack some newer APIs without matching a specific version of Windows precisely, thus probably requiring dynamic fallbacks.
I would expect it to not have a separate target, because the goal of the project is to run Windows software unmodified.
(I don't know what exact APIs they lack now, but a few years ago they were somewhere around Vista, and the development was pretty fast in the last couple of years.)

@crlf0710
Copy link
Member

crlf0710 commented Nov 28, 2019

I think the split is mainly trying to reflect

  1. the tier-1 state of current windows implementation support, which is ok for win7+, but doesn't support windows xp target well;
  2. and the tier-3 state of windows xp fallback, where some users i know of demanded it (for developing in-house software), but was never satisfied. Some of the most obvious blocking issues were:
    ** extends windows subsystem support (or just relax limitation) #37553 (comment) (the generated executable doesn't set the correct minimal system version, which is already addressed by this PR)
    ** Investigate fallback implementations on Windows XP #26654, the most broken one is rwlock, which is unusable at all on xp. And if i remembered correctly there might be some stuff in std::fs that works sometimes and sometimes not.
    ** This broken rwlock in turns blocks the whole panicking support, in Panic broken on Windows XP #34538

I don't really know the current status of ReactOS, but I think at least ReactOS can use tier-3 xp target when the issues above are solved.

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Nov 28, 2019

Incidentally when parking_lot lands I think it will replace the platform rwlock. So with this PR (or something like it) and parking_lot, XP support will improve as a by-product. Also having an explicit XP target will also allow crates to offer XP fallbacks (although given the lack of interested in supporting XP I have doubts that many will).

More broadly I agree with crlf0710 that splitting the targets reflects the actual split between a tier-1 Windows 7+ target and tier-3 (to be generous) XP target that currently exists in practice. Having them tied together unnecessarily complicates supporting both. Note that the XP "fallbacks" don't do anything useful other than prevent some linking errors. You just get unexpected failures at runtime if you happen to use a libstd function that calls the API.

@mati865
Copy link
Contributor

mati865 commented Nov 28, 2019

MinGW support isn't that easy because winpthread library could be compiled targetting newer systems (MSYS2 mingw-w64 targets Windows 7 for an example, see msys2/MSYS2-packages#1784).

@retep998
Copy link
Member

There's also the wider discussion of how to handle targeting specific versions of an OS in general. There are plenty of features added in many versions of Windows that crates would like to know whether they can rely on or have to support dynamic fallback for.

@ChrisDenton
Copy link
Member Author

@retep998 yeah, I'm trying to avoid over complicating this particular PR because I think that's a bigger issue and there is still value in treating XP as a separate platform even if it's solved. Visual Studio treats XP somewhat specially (and btw, marks it deprecated now) so this is not without precedent.

For what it's worth I did consider adding something like a windows_environment structure that would include:

  • Platform toolset
  • Windows SDK version
  • Application manifest
  • Subsystem:
    • Name
    • Minimum version

But this would be a major change and would probably need a lot more discussion. And I'm still getting to grips with the moving parts of Rust's build system.

@alexcrichton
Copy link
Member

I can personally review the technical implementation details here, but I would not be comfortable being a sole person signing off on inclusion of these targets. This is pretty different from all our other targets since we don't really have all that many targets which are the same except for OS version.

@joshtriplett with your recent work on tiers and such, would you be able to help out in reviewing the addition of these two targets? (in concept, not in terms of the technical details)

@ChrisDenton
Copy link
Member Author

That sounds fair. My argument would be that we do already have a separate UWP target so it's not unprecedented. And the current pc-windows-msvc targets are the only ones to my knowledge that target both Tier 1 and Tier 3 OSes. I think a new target would be the least invasive option if/when XP is dropped completely from official support.

That said another option would be to make new more generic platform version information available from some kind of config option. XP would need to be special cased though, as it is in VS.

@joshtriplett
Copy link
Member

@alexcrichton This seems entirely reasonable to me in principle, and the code changes look reasonable as well.

Firefox no longer supports Windows XP, and I don't know of any other projects still attempting to support XP, so splitting this out and declaring it a tier 3 target seems like a reasonable step.

I do wonder where the dividing line should be; I've seen proposals to put Vista support in this bucket as well, which seems similarly reasonable as Vista has also been EOLed and no longer has security support.

I do not want to derail this, but as a side note, I'd also love to see a "Windows 10 and newer only" target, which could then make many more assumptions about available APIs and capabilities, especially when it comes to POSIX compatibility.

But let's not let that distract from the proposal to separate out XP support.

@retep998
Copy link
Member

retep998 commented Dec 3, 2019

I do think we should at least think about how this would be part of a future world where we have multiple targets for different minimum Windows versions. What sort of naming convention would we follow and what would vista+ or 7+ or 10+ targets be called?

@crlf0710
Copy link
Member

crlf0710 commented Dec 3, 2019

Future marketing strategy is impossible to predict, but if you want a consistent naming, maybe something like this https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions :

  • xp+ => nt5x (because it's a combination of nt51(32bit) and nt52(64bit))
  • vista+ => nt60
  • 7+ => nt61
  • 8+ => nt62
  • 8.1+ => nt63
  • 10+ => nt100

With that said, maybe it makes no sense to add any of these to the "default" windows target, maybe add one of these tags when such a target falls off tier 1 level, i think

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Dec 3, 2019

IMHO, I don't think Rust has ever officially supported Vista (even if it worked)? Windows 7+ is supported and I think the Firefox people were the main advocates for limited XP support but there was no Vista policy. In any case, I feel that Vista+ versions of Windows are similar enough that new targets for each would not be necessary.

But yes, I think this is a long standing important issue but drifting off the topic of this PR. My preference would, as I briefly hinted at above, be to have some kind cfg value that holds the Windows SDK version being targetted. So something like 7.1, 8.1 and 10.0.18362.0. Then you just need a way to test it's above or equal to whatever minimum you want. Testing modern Windows versions by name is less useful, I think. Especially with Windows 10 being the supposed "last" version of Windows.

Maybe this should be a new issue or forum discussion?

@alexcrichton
Copy link
Member

AFAIK we do not have precedent for different targets for different OS versions. We do have different targets for different runtimes/libc implementations (e.g. gnu/musl, msvc/gnu, msvc/uwp, etc), and it's not like we can't have different implementations for different versions of an OS, we just haven't really had a need come up since we've always dealt with it in code and that's been a satisfactory solution for the time beign.

That's just some thoughts though, I'd prefer to defer to a consensus of a group that we should add these targets.

@ChrisDenton
Copy link
Member Author

That's fair, I'm not arguing against reaching a consensus whatever the outcome. I'm just clumsily trying to make my reasons clear. Which I've done, so I'll wait to see what decision is made.

@tesuji
Copy link
Contributor

tesuji commented Dec 11, 2019

So the Rust team is in favor of <rust-lang/rfcs#2837>. We could have the same consensus
for Windows XP targets?

@est31
Copy link
Member

est31 commented Dec 11, 2019

The manufacturer situation is similar: Apple removed 32 bit support in Xcode 10, while MS removed XP support in VS 2019. So one would similarly require CI providers to keep support for VS 2017.

But there is still a significant user base running on XP. According to netmarketshare it's 2.29% of the Desktop pie, more than Linux or semi-old Mac OS versions.

OS Desktop marketshare
Windows 10 46.59%
Windows 7 33.37%
Mac OS X 10.14 5.24%
Windows 8.1 4.15%
Windows XP 2.29%
Mac OS X 10.13 2.07%
Linux 1.47%
Mac OS X 10.12 0.96%
Windows 8 0.71%
Mac OS X 10.11 0.63%

Furthermore, even if XP had lower market share, it's still running on countless legacy systems around the world. Those systems are in our power plants, MRI scanners, manufacturing machines, aircraft carriers, etc. Maybe not all of those systems are being served updates, but I guess some are. It would be sad if you couldn't put Rust there.

Apple hardware on the other hand isn't really deployed in those places that require long-term support.

@joshtriplett
Copy link
Member

The proposal is to separate XP support, not to immediately drop that support.

@ChrisDenton
Copy link
Member Author

Yes a separate XP target wouldn't remove XP support. In fact it may even improve it, especially if someone were willing to actively maintain it. At the minute you have to go through a bit of effort to produce even a hello world app that runs on XP.

Btw, since April 9, 2019 there is no version of XP that's supported by MS, even for paying customers. This does make supporting it harder. However, if there are still people willing to patch up Rust for XP then having a target to focus their contributions on can only help.

@est31
Copy link
Member

est31 commented Dec 11, 2019

@joshtriplett I was replying to @luzato and interpreting "same consensus" as eventual removal of the XP target (the RFC is very much in that direction even though it doesn't remove the support fully yet). I'm not in favour of XP removal, at least not at this point. A separate XP target is a good idea.

@bors
Copy link
Contributor

bors commented Dec 23, 2019

☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts.

Adds explicit targets for Windows XP.
XP requires an older subsystem to be used than is the default.
This reverts commit 3f8c0da415824be941292981ea40f115a94f0d97.
XP requires an older subsystem to be used than is the default. To support this, an optional version parameter has been added to the Linker `subsystem` function and a WindowsSubsystem struct to CodegenResults.
@crlf0710
Copy link
Member

crlf0710 commented Jan 7, 2020

I think the best way forward here is to start a fcp, and proceed when consensus is reached? What do you think? @pietroalbini who started the fcp in rfc2837

cc @rust-lang/release @rust-lang/compiler
cc @rust-lang/infra

@pietroalbini
Copy link
Member

I'll defer what to do here to @alexcrichton.

@alexcrichton
Copy link
Member

I'm not prepared, nor can I at this time, lead a proposal to add this target to the compiler. The building of consensus, or the discussion of how to build consensus, about adding XP targets to the compiler will need to be led by someone else. I believe such consensus, however, is the next step here.

@retep998
Copy link
Member

While I am in favor of being able to distinguish building for different versions of Windows, I don't think that overloading the target vendor for that is the correct way to do it. I am neutral on whether it should be a new target or another mechanism, but whatever it is, it should be flexible enough that we can easily extend it to any version of Windows.

Also whatever we end up doing, it should allow the user to write code that does "Am I targeting at least Windows XP" and not "Am I targeting exactly Windows XP". Having a cfg on target_vendor would be the latter and would result in version compatibility issues every time a new version of Windows is added as a possible option.

@ChrisDenton
Copy link
Member Author

Yeah, I'm not suggesting this as a general solution to targetting Windows versions. I have some thoughts on that and I think Vista+ will be well suited to using whatever mechanism that ends up being.

However I think we do need something that says "Am I targetting exactly XP" because compiling for XP is so different, quite apart from available APIs etc (and even there there can be subtle incompatibilities). Ideally XP needs a particular msvc toolset as well as the linker options and more. This PR currently does the bare minimum just to get something that will sort of run. assuming panics aren't used and you avoid other pitfalls. If, hypothetically, XP were a tier one target I'm not sure how it could be fully supported with "Am I targeting at least Windows XP" alone.

In short, I think XP is sufficiently special to warrant special treatment in some way, even if not using target_vendor. The absolute least that's needed is for special instructions to be passed to the linker for handling XP builds. As far as I understand it the differences in Vista+ versions of Windows can, in principle, be handled purely in std?

@retep998
Copy link
Member

Handling XP specific toolchain stuff can be done by checking and(at_least_xp, not(at_least_vista)) and anything that checks the target should be able to check whatever we come up with for specifying the Windows version. There's a reason the officially recommended way to check your Windows version programmatically is via functions like IsWindowsXPOrGreater.

@ChrisDenton
Copy link
Member Author

Right, but my point is there is a need to target XP specifically (in whatever way) and more importantly that it needs to be checked in a different situation to Win 7+, simply to compile a working binary. I feel like I'm not explaining myself very well so I'll take the example of how Microsoft Visual C++ does it.

To make a new C++ Windows 7+ application in Visual Studio 2019 all you have to do is create a new project and build it. You don't need to do anything particularly special, aside perhaps making sure you call the right OS functions in the right way (this is where versionhelper macros can sometimes be useful).

However, to make a new C++ Windows XP application you first need to install the VS 2017 XP toolset.

winxp_install

Then you have to go to your project and change it to use that specially modified toolset.

winxp_use

This sets necessary options as well as linking older platform libraries,

So with Windows 7+ the build environment remains the same across Windows versions. Changes are optional and for the most part confined to how the application calls system functions. With Windows XP (unlike 7SP1, 8.1, 10) you need a different build environment.

Could Rust use a cfg option to tell cargo and the compiler to build for XP? Sure. But my point is we'd still be treating XP in a different way because it wouldn't just be std and user code that needs to know that it's a special kind of Windows.

I'm not insisting that Windows XP needs to be an explicit target, but I am saying it needs to be built differently than all the versions of Windows that come after it.

@ChrisDenton
Copy link
Member Author

Thinking about it, one alternative might be to mirror Visual Studio more closely. Rust could have a windows_toolchain option which, as in VS, is separate from any Windows version values. Cargo can interpret that to find the right libs before passing it on to rustc which can use it to set the correct linker options.

@ChrisDenton
Copy link
Member Author

Having discussed this with nagisa, I don't think there's a way forward with this PR in its current form. There are alternatives, such as adding an optional version parameter to the windows_subsystem attribute. This could also be used as a hint by the compiler to attempt to find the correct Windows SDK version for the subsystem. VS and the official Windows SDK installers puts the paths in the registry so they're findable in a similar way as MSVC compiler and tools are. In addition there could be some way to tell rust to try to use a specific SDK version, and rustc will try to automatically resolve the library search path.

Some of the linker interface code in this PR could be reused in other proposals but those will need discussions of their own. So I think this should be closed.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.