Add UWP MSVC targets #63155
Merged
Add UWP MSVC targets #63155
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
JohnTitor
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @zackmdavis (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. |
Centril
added
relnotes
| // FIXME: this shouldn't be panic=abort, it should be panic=unwind | ||
| base.panic_strategy = PanicStrategy::Abort; | ||
|
|
||
| let lib_root_path = env::var("VCToolsInstallDir") |
There was a problem hiding this comment.
Note that we already have a good deal of support for this via the cc::windows_registry probing that we do in the backend. I think it'd be best to do this there instead of here to not require that the VCToolsInstallDir env var is set. Current MSVC targets don't set this env var and I think that with the probing logic we have you should be able to find the root path anyway?
alexcrichton
reviewed
Aug 5, 2019
src/librustc_target/Cargo.toml
Outdated
| @@ -11,6 +11,7 @@ path = "lib.rs" | |||
| [dependencies] | |||
| bitflags = "1.0" | |||
| log = "0.4" | |||
| cc = "1.0.1" | |||
There was a problem hiding this comment.
Oh sorry I meant moreso that cc is laready linked elsewhere and it's already handled in probing for it, so we should reuse that location to add paths to link as opposed to doing so in the target spec directory here. (as this is how MSVC is handled in general)
There was a problem hiding this comment.
Sorry I'm still not clear on what to do.
You want me to patch cc-rs or librustc_codegen_ssa or both?
There was a problem hiding this comment.
Sorry so what I mean is that librustc_codegen_ssa already (AFAIK) has tons of logic for dealing with MSVC and leverages the cc crate to find compilers and add paths to link against. The logic for handling UWP paths should all happen around there rather than being duplicated here.
There was a problem hiding this comment.
Code is not pretty... but it seems to work. Let me know what you think
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
jdm
reviewed
Aug 8, 2019
| base.max_atomic_width = Some(64); | ||
| base.has_elf_tls = true; | ||
|
|
||
| // FIXME: this shouldn't be panic=abort, it should be panic=unwind |
There was a problem hiding this comment.
What's the background on this FIXME that causes it to exist? Does that belong in a comment or an issue?
There was a problem hiding this comment.
The AArch64 Windows backend didn't implement SEH unwinding when the aarch64-pc-windows-msvc target was first added, so I suspect this is the same as that. I haven't checked the LLVM backend recently to see if this works.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
alexcrichton
reviewed
Aug 8, 2019
| @@ -1027,6 +1027,20 @@ fn link_args<'a, B: ArchiveBuilder<'a>>(cmd: &mut dyn Linker, | |||
| let t = &sess.target.target; | |||
|
|
|||
| cmd.include_path(&fix_windows_verbatim_for_gcc(&lib_path)); | |||
|
|
|||
| if t.linker_flavor == LinkerFlavor::Msvc && t.target_vendor == "uwp" { | |||
There was a problem hiding this comment.
Can this happen inside get_linker where we're already executing find_tool?
|
|
||
| if t.linker_flavor == LinkerFlavor::Msvc && t.target_vendor == "uwp" { | ||
| let link_tool = windows_registry::find_tool("x86_64-pc-windows-msvc", "link.exe") | ||
| .expect("no path found for link.exe"); |
There was a problem hiding this comment.
This should report errors as first-class errors instead of panics, but for this case I think that all errors should be swallowed and this logic ignored. This could perhaps warn! on errors but it's probably not worthwhile to halt compilation if this fails.
| .expect("no path found for link.exe"); | ||
|
|
||
| let original_path = link_tool.path(); | ||
| let root_lib_path = original_path.ancestors().skip(4).next().unwrap(); |
There was a problem hiding this comment.
Like above this'll want to have error handling to avoid the unwrap
| base.max_atomic_width = Some(64); | ||
| base.has_elf_tls = true; | ||
|
|
||
| // FIXME: this shouldn't be panic=abort, it should be panic=unwind |
There was a problem hiding this comment.
The AArch64 Windows backend didn't implement SEH unwinding when the aarch64-pc-windows-msvc target was first added, so I suspect this is the same as that. I haven't checked the LLVM backend recently to see if this works.
| @@ -158,6 +159,33 @@ pub fn get_linker(sess: &Session, linker: &Path, flavor: LinkerFlavor) -> (PathB | |||
| } | |||
| }; | |||
|
|
|||
| let t = &sess.target.target; | |||
| if t.linker_flavor == LinkerFlavor::Msvc && t.target_vendor == "uwp" { | |||
There was a problem hiding this comment.
This should read the flavor local variable instead of looking this up again
|
@bors: r+ |
|
📌 Commit 1581c43 has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 15, 2019
Add UWP MSVC targets Hi, - The README URI change is the correct one for VS2019 community edition, which I suspect most people would use. Doesn't _need_ to be merged though. - This rust-lang@5e6619e fixes the UWP build (msvc or not, doesn't matter). I suspect it broke with recent changes unnoticed because no CI. - Store lib location is found through the VCToolsInstallDir env variable. The end of the path is currently for the VS2019 store lib locations only. - I could not test the aarch64_uwp_windows_msvc target because the rust build script does not currently support arm64 msvc AFAIU.
|
⌛ Testing commit 1581c43 with merge d321c2ffd8513b4084e3fa20d2c19931985dc450... |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 15, 2019
Add UWP MSVC targets Hi, - The README URI change is the correct one for VS2019 community edition, which I suspect most people would use. Doesn't _need_ to be merged though. - This rust-lang@5e6619e fixes the UWP build (msvc or not, doesn't matter). I suspect it broke with recent changes unnoticed because no CI. - Store lib location is found through the VCToolsInstallDir env variable. The end of the path is currently for the VS2019 store lib locations only. - I could not test the aarch64_uwp_windows_msvc target because the rust build script does not currently support arm64 msvc AFAIU.
|
@bors retry rolled up. |
|
⌛ Testing commit 1581c43 with merge 3f1dd30b59a482e205175bae45d411643bd226b8... |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 15, 2019
Add UWP MSVC targets Hi, - The README URI change is the correct one for VS2019 community edition, which I suspect most people would use. Doesn't _need_ to be merged though. - This rust-lang@5e6619e fixes the UWP build (msvc or not, doesn't matter). I suspect it broke with recent changes unnoticed because no CI. - Store lib location is found through the VCToolsInstallDir env variable. The end of the path is currently for the VS2019 store lib locations only. - I could not test the aarch64_uwp_windows_msvc target because the rust build script does not currently support arm64 msvc AFAIU.
|
@bors retry rolled up. |
bors
added a commit
that referenced
this pull request
Aug 15, 2019
Rollup of 9 pull requests Successful merges: - #63155 (Add UWP MSVC targets) - #63165 (Add builtin targets for mips64(el)-unknown-linux-muslabi64) - #63306 (Adapt AddRetag for shallow retagging) - #63467 (Add Catalyst (iOS apps running on macOS) target) - #63546 (Remove uses of `mem::uninitialized()` from cloudabi) - #63572 (remove unused Level::PhaseFatal) - #63577 (Test HRTB issue accepted by compiler) - #63582 (Fix ICE #63226) - #63586 (cleanup: Remove `Spanned` where possible) Failed merges: r? @ghost
|
Woo! Thanks for the work to enable this target, and the work to review it everyone! |
This was referenced Dec 31, 2019
bors
added a commit
to rust-lang/cargo
that referenced
this pull request
Jan 6, 2020
…excrichton Fix dynamic linking for Windows UWP MSVC targets When creating a dynamic library, the MSVC linker generates an import library (.lib) next to the .dll file. Cargo has explicit knowledge of this and includes those generated .dll.lib on the list of files generated by a Cargo invocation. However, the check to see if those import libraries must be included is too strict and doesn't match any Windows targets that don't end in `pc-windows-msvc`. For example, rust-lang/rust#63155 added several new Windows targets for targeting UWP called `*-uwp-windows-msvc`. The end result is that the sysroot for these UWP toolchains don't contain a `std-XXX.dll.lib` file and thus any executable that uses `-C prefer-dynamic` will fail to link because the `std` library is not linked at all. This change relaxes the test and makes Cargo know about those import libraries for all Windows MSVC targets.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,