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 Windows application manifest to rustc-main #96737

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

ChrisDenton
Copy link
Contributor

@ChrisDenton ChrisDenton commented May 5, 2022

Windows allows setting some runtime options using a manifest file. The format of the XML file is documented here: https://docs.microsoft.com/en-us/windows/win32/sbscs/application-manifests

The manifest file in this PR does three things:

  • Declares which Windows versions we support. This may help avoid unnecessary compatibility shims.
  • Uses the UTF-8 code page. While Rust itself uses UTF-16 APIs, other code may rely on the code page.
  • Makes the application long path aware (if also enabled by the user). This allows for the current directory to be longer than PATH_MAX.

These changes only affect the rustc process and not any other DLLs or compiled programs.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 5, 2022
@rust-highfive
Copy link
Collaborator

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 5, 2022
compiler/rustc/build.rs Outdated Show resolved Hide resolved
compiler/rustc/build.rs Outdated Show resolved Hide resolved
compiler/rustc/build.rs Outdated Show resolved Hide resolved
Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

I'm not terribly familiar with how windows works, but it seems fine.

@cjgillot
Copy link
Contributor

cjgillot commented May 7, 2022

I have absolutely no knowledge of windows.
r? @bjorn3 (or do we have a resident windows expert?)

@rust-highfive rust-highfive assigned bjorn3 and unassigned cjgillot May 7, 2022
@bjorn3
Copy link
Member

bjorn3 commented May 7, 2022

I'm afraid this could have unintended effects I don't know about. Going to reassign to @wesleywiser as they are a member of both the windows notification group and the compiler team.

r? @wesleywiser

@rust-highfive rust-highfive assigned wesleywiser and unassigned bjorn3 May 7, 2022
@AronParker
Copy link
Contributor

AronParker commented May 7, 2022

I believe /MANIFESTUAC:"level='asInvoker'" should be in there for two reasons:

  1. Windows tries to guess whether programs require elevation based on filename, if the scan reveals the executable is named "install", "setup" or "update", it automatically requires elevation as described here. We don't need this check for the rust compiler.
  2. There are some weird backwards compatibility hacks from the XP era where everything ran as administrator, one of them is virtualizing the HKEY_LOCAL_MACHINE hive or folders that were previously accessible such as Program Files as described here, we definitely don't need this for rustc and when it tries to write in such locations it should fail.

@bjorn3
Copy link
Member

bjorn3 commented May 7, 2022

The installer detection only applies to 32bit programs and I'm pretty sure the heuristics for it don't trigger on rustc.exe. We would have gotten complaints about it otherwise.

@AronParker
Copy link
Contributor

The installer detection only applies to 32bit programs and I'm pretty sure the heuristics for it don't trigger on rustc.exe. We would have gotten complaints about it otherwise.

Do we not offer compilation support for 32 bit Windows as well? And clearly they won't, but checking them might not be necessary either. I don't have a strong opinion on it either way, just my two cents.

@retep998
Copy link
Member

retep998 commented May 10, 2022

We absolutely should provide a manifest, and I see zero downsides to doing so (other than the complexity of implementing this). The only question is the specific technical details of how.

I do wish we had better generalized support for embedding manifests into Rust applications, and if we ever do get that the code added here should be updated to use that.

<!-- Remove (most) legacy path limits -->
<asmv3:application>
<asmv3:windowsSettings xmlns:ws2="http://schemas.microsoft.com/SMI/2016/WindowsSettings">
<ws2:longPathAware>true</ws2:longPathAware>
Copy link
Member

Choose a reason for hiding this comment

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

While this only takes effect if the system has long paths enabled on it (it is disabled by default), there is no downside to this so long as rustc has no issues with handling long paths (which Rust itself is absolutely fine with and the only concern would be specific C/C++ dependencies we have like LLVM but I'm pretty sure that's also fine).

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this potentially change the way that rustc emits paths in error messages (i.e., using the \\?\ prefixed syntax)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it should not change anything there. It allows C:\... paths to be used beyond the 260 limit without the \\?\ prefix but does not otherwise alter how rustc sees paths.

<!-- Use UTF-8 code page -->
<asmv3:application>
<asmv3:windowsSettings xmlns="http://schemas.microsoft.com/SMI/2019/WindowsSettings">
<activeCodePage>UTF-8</activeCodePage>
Copy link
Member

Choose a reason for hiding this comment

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

This only affects A suffixed Windows API usage, which Rust doesn't use anywhere. Rustc may have C/C++ dependencies which do however. The best way to be certain about this is to check whether there are any conversions between unicode and the narrow system encoding. Most like we're completely in the clear and this change would only improve things.

println!("cargo:rustc-link-arg-bin=rustc-main=/MANIFEST:EMBED");
println!("cargo:rustc-link-arg-bin=rustc-main=/MANIFESTINPUT:{}", manifest.to_str().unwrap());
// Turn linker warnings into errors.
println!("cargo:rustc-link-arg-bin=rustc-main=/WX");
Copy link
Member

@retep998 retep998 May 10, 2022

Choose a reason for hiding this comment

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

To be honest, I wish we enabled this more often. Rust currently hides all output from the linker if it succeeds so linker warnings are completely not noticed, even if they may indicate something is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye, My specific motivation here was to catch manifest related warnings that are actually important. But I kinda think it's something that should be on by default for most projects unless they have a specific need to disable it (e.g. legacy code, etc).

@ChrisDenton
Copy link
Contributor Author

I do wish we had better generalized support for embedding manifests into Rust applications, and if we ever do get that the code added here should be updated to use that.

Cargo recently added rustc-link-arg-* options which is what now makes it slightly simpler to add a manifest file with just a build script. But only if using an msvc linker (or compatible) as gnu does not have the manifest options. So proper support would indeed be great.

</asmv3:application>
<!-- Remove (most) legacy path limits -->
<asmv3:application>
<asmv3:windowsSettings xmlns:ws2="http://schemas.microsoft.com/SMI/2016/WindowsSettings">
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking this PR, but it would be worth testing if setting heapType provides any perf benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that would be interesting! But yes, I would prefer that to be a separate PR because of the perf differences.

Copy link
Contributor

@klensy klensy Jun 1, 2022

Choose a reason for hiding this comment

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

Indeed, that would be interesting! But yes, I would prefer that to be a separate PR because of the perf differences.

But rust infra only have linux perf runner and not windows, so how to see this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it can be run locally or with a try build if you edit the configuration also. I'm not sure though.

In any case, I think that would be a problem for another PR rather than this one.

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2022

📌 Commit 7255bc6 has been approved by wesleywiser

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 2, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 2, 2022
@bors
Copy link
Contributor

bors commented Jun 2, 2022

⌛ Testing commit 7255bc6 with merge 5e6bb83...

@bors
Copy link
Contributor

bors commented Jun 2, 2022

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 5e6bb83 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 2, 2022
@bors bors merged commit 5e6bb83 into rust-lang:master Jun 2, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 2, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5e6bb83): comparison url.

Instruction count

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-0.7% -0.9% 7
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
2.6% 2.6% 1
Regressions 😿
(secondary)
2.7% 2.8% 3
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 2.6% 2.6% 1

Cycles

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
2.2% 2.3% 2
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.8% -2.8% 1
All 😿🎉 (primary) 2.2% 2.3% 2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

Copy link

@Locomania69 Locomania69 left a comment

Choose a reason for hiding this comment

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

Nopuedo conetarme

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 10, 2022
Pkgsrc changes:
 * Adjust patches as needed & checksum updates.

Upstream changes:

Version 1.63.0 (2022-08-11)
==========================

Language
--------
- [Remove migrate borrowck mode for pre-NLL errors.][95565]
- [Modify MIR building to drop repeat expressions with length zero.][95953]
- [Remove label/lifetime shadowing warnings.][96296]
- [Allow explicit generic arguments in the presence of `impl Trait` args.]
  [96868]
- [Make `cenum_impl_drop_cast` warnings deny-by-default.][97652]
- [Prevent unwinding when `-C panic=abort` is used regardless of
  declared ABI.][96959]
- [lub: don't bail out due to empty binders.][97867]

Compiler
--------
- [Stabilize the `bundle` native library modifier,][95818] also removing the
  deprecated `static-nobundle` linking kind.
- [Add Apple WatchOS compile targets\*.][95243]
- [Add a Windows application manifest to rustc-main.][96737]

\* Refer to Rust's [platform support page][platform-support-doc] for more
   information on Rust's tiered platform support.

Libraries
---------
- [Implement `Copy`, `Clone`, `PartialEq` and `Eq` for
  `core::fmt::Alignment`.][94530]
- [Extend `ptr::null` and `null_mut` to all thin (including extern)
  types.][94954]
- [`impl Read and Write for VecDeque<u8>`.][95632]
- [STD support for the Nintendo 3DS.][95897]
- [Make write/print macros eagerly drop temporaries.][96455]
- [Implement internal traits that enable `[OsStr]::join`.][96881]
- [Implement `Hash` for `core::alloc::Layout`.][97034]
- [Add capacity documentation for `OsString`.][97202]
- [Put a bound on collection misbehavior.][97316]
- [Make `std::mem::needs_drop` accept `?Sized`.][97675]
- [`impl Termination for Infallible` and then make the `Result` impls
  of `Termination` more generic.][97803]
- [Document Rust's stance on `/proc/self/mem`.][97837]

Stabilized APIs
---------------

- [`array::from_fn`]
- [`Box::into_pin`]
- [`BinaryHeap::try_reserve`]
- [`BinaryHeap::try_reserve_exact`]
- [`OsString::try_reserve`]
- [`OsString::try_reserve_exact`]
- [`PathBuf::try_reserve`]
- [`PathBuf::try_reserve_exact`]
- [`Path::try_exists`]
- [`Ref::filter_map`]
- [`RefMut::filter_map`]
- [`NonNull::<[T]>::len`][`NonNull::<slice>::len`]
- [`ToOwned::clone_into`]
- [`Ipv6Addr::to_ipv4_mapped`]
- [`unix::io::AsFd`]
- [`unix::io::BorrowedFd<'fd>`]
- [`unix::io::OwnedFd`]
- [`windows::io::AsHandle`]
- [`windows::io::BorrowedHandle<'handle>`]
- [`windows::io::OwnedHandle`]
- [`windows::io::HandleOrInvalid`]
- [`windows::io::HandleOrNull`]
- [`windows::io::InvalidHandleError`]
- [`windows::io::NullHandleError`]
- [`windows::io::AsSocket`]
- [`windows::io::BorrowedSocket<'handle>`]
- [`windows::io::OwnedSocket`]
- [`thread::scope`]
- [`thread::Scope`]
- [`thread::ScopedJoinHandle`]

These APIs are now usable in const contexts:

- [`array::from_ref`]
- [`slice::from_ref`]
- [`intrinsics::copy`]
- [`intrinsics::copy_nonoverlapping`]
- [`<*const T>::copy_to`]
- [`<*const T>::copy_to_nonoverlapping`]
- [`<*mut T>::copy_to`]
- [`<*mut T>::copy_to_nonoverlapping`]
- [`<*mut T>::copy_from`]
- [`<*mut T>::copy_from_nonoverlapping`]
- [`str::from_utf8`]
- [`Utf8Error::error_len`]
- [`Utf8Error::valid_up_to`]
- [`Condvar::new`]
- [`Mutex::new`]
- [`RwLock::new`]

Cargo
-----
- [Stabilize the `--config path` command-line argument.][cargo/10755]
- [Expose rust-version in the environment as
  `CARGO_PKG_RUST_VERSION`.][cargo/10713]

Compatibility Notes
-------------------

- [`#[link]` attributes are now checked more strictly,][96885]
  which may introduce errors for invalid attribute arguments that
  were previously ignored.

Internal Changes
----------------

These changes provide no direct user facing benefits, but represent
significant improvements to the internals and overall performance
of rustc and related tools.

- [Prepare Rust for LLVM opaque pointers.][94214]

[94214]: rust-lang/rust#94214
[94530]: rust-lang/rust#94530
[94954]: rust-lang/rust#94954
[95243]: rust-lang/rust#95243
[95565]: rust-lang/rust#95565
[95632]: rust-lang/rust#95632
[95818]: rust-lang/rust#95818
[95897]: rust-lang/rust#95897
[95953]: rust-lang/rust#95953
[96296]: rust-lang/rust#96296
[96455]: rust-lang/rust#96455
[96737]: rust-lang/rust#96737
[96868]: rust-lang/rust#96868
[96881]: rust-lang/rust#96881
[96885]: rust-lang/rust#96885
[96959]: rust-lang/rust#96959
[97034]: rust-lang/rust#97034
[97202]: rust-lang/rust#97202
[97316]: rust-lang/rust#97316
[97652]: rust-lang/rust#97652
[97675]: rust-lang/rust#97675
[97803]: rust-lang/rust#97803
[97837]: rust-lang/rust#97837
[97867]: rust-lang/rust#97867
[cargo/10713]: rust-lang/cargo#10713
[cargo/10755]: rust-lang/cargo#10755

[`array::from_fn`]: https://doc.rust-lang.org/stable/std/array/fn.from_fn.html
[`Box::into_pin`]: https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.into_pin
[`BinaryHeap::try_reserve_exact`]: https://doc.rust-lang.org/stable/alloc/collections/binary_heap/struct.BinaryHeap.html#method.try_reserve_exact
[`BinaryHeap::try_reserve`]: https://doc.rust-lang.org/stable/std/collections/struct.BinaryHeap.html#method.try_reserve
[`OsString::try_reserve`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve
[`OsString::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve_exact
[`PathBuf::try_reserve`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve
[`PathBuf::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve_exact
[`Path::try_exists`]: https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.try_exists
[`Ref::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.Ref.html#method.filter_map
[`RefMut::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.RefMut.html#method.filter_map
[`NonNull::<slice>::len`]: https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.len
[`ToOwned::clone_into`]: https://doc.rust-lang.org/stable/std/borrow/trait.ToOwned.html#method.clone_into
[`Ipv6Addr::to_ipv4_mapped`]: https://doc.rust-lang.org/stable/std/net/struct.Ipv6Addr.html#method.to_ipv4_mapped
[`unix::io::AsFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/trait.AsFd.html
[`unix::io::BorrowedFd<'fd>`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.BorrowedFd.html
[`unix::io::OwnedFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.OwnedFd.html
[`windows::io::AsHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsHandle.html
[`windows::io::BorrowedHandle<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedHandle.html
[`windows::io::OwnedHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedHandle.html
[`windows::io::HandleOrInvalid`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrInvalid.html
[`windows::io::HandleOrNull`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrNull.html
[`windows::io::InvalidHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.InvalidHandleError.html
[`windows::io::NullHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.NullHandleError.html
[`windows::io::AsSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsSocket.html
[`windows::io::BorrowedSocket<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedSocket.html
[`windows::io::OwnedSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedSocket.html
[`thread::scope`]: https://doc.rust-lang.org/stable/std/thread/fn.scope.html
[`thread::Scope`]: https://doc.rust-lang.org/stable/std/thread/struct.Scope.html
[`thread::ScopedJoinHandle`]: https://doc.rust-lang.org/stable/std/thread/struct.ScopedJoinHandle.html

[`array::from_ref`]: https://doc.rust-lang.org/stable/std/array/fn.from_ref.html
[`slice::from_ref`]: https://doc.rust-lang.org/stable/std/slice/fn.from_ref.html
[`intrinsics::copy`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy.html
[`intrinsics::copy_nonoverlapping`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy_nonoverlapping.html
[`<*const T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to
[`<*const T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping
[`<*mut T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to-1
[`<*mut T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping-1
[`<*mut T>::copy_from`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from
[`<*mut T>::copy_from_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from_nonoverlapping
[`str::from_utf8`]: https://doc.rust-lang.org/stable/std/str/fn.from_utf8.html
[`Utf8Error::error_len`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.error_len
[`Utf8Error::valid_up_to`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.valid_up_to
[`Condvar::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html#method.new
[`Mutex::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.new
[`RwLock::new`]: https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.new
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet