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

Windows library loading #3493

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Windows library loading #3493

merged 1 commit into from
Oct 5, 2023

Conversation

ChrisDenton
Copy link
Member

When loading DLLs, the loader uses a standard search order that may include the application directory. To ensure the right DLLs get loaded, this PR makes sure to force DLLs to be loaded from system32. This is done using /DEPENDENTLOADFLAG msvc linker option. However this only works for Windows 10 RS1 and higher (which admittedly is almost all Windows 10 users) and will have no effect on earlier OSes. So as a fallback we use the delay load mechanism to get the DLLs loaded at runtime so that we can use SetDefaultDllDirectories to affect DLL loading. Note that DLLs on the system's list of "Known DLLs" are always loaded from system32 so don't need to be delay loaded this way.

use winapi::um::libloaderapi::{SetDefaultDllDirectories, LOAD_LIBRARY_SEARCH_SYSTEM32};
// Default to loading delay loaded DLLs from the system directory.
unsafe {
let result = SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32);
Copy link
Member

Choose a reason for hiding this comment

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

The docs for this function seem to say that it's not directly available on Win7:

Windows 7, Windows Server 2008 R2, Windows Vista and Windows Server 2008: To call this function in an application, use the GetProcAddress function to retrieve its address from Kernel32.dll. KB2533623 must be installed on the target platform.

So... how did this call work? Are we not setting up the build in a Win7 mode? (until we actually drop that support)

Copy link
Member Author

@ChrisDenton ChrisDenton Sep 28, 2023

Choose a reason for hiding this comment

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

Hm, this works in my Win7 VM. Presumably the advice to use GetProcAddress is because the old SDK libraries didn't include them and they require an OS update to work. You get an error message if the OS update is not installed but it's quite easy to find answers on the web pointing to the necessary update. I think it's ok to require users to have installed security updates, especially for such an old OS.

But yes, Windows 7 hasn't been tested in CI in a long time. The same goes for all rustlang repos I'm aware of.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the Rust 1.72 blog post had this text:

In a future release we're planning to increase the minimum supported Windows version to 10. The accepted proposal in compiler MCP 651 is that Rust 1.75 will be the last to officially support Windows 7, 8, and 8.1. When Rust 1.76 is released in February 2024, only Windows 10 and later will be supported as tier-1 targets. This change will apply both as a host compiler and as a compilation target.

How do you think those changes (5 months from now) should interact with this code?

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't change anything until rust's minimum supported version is Windows 10 RS1 (aka 1607). I can add a similar comment to the one in the build script and note how they're related.

Copy link
Contributor

@djc djc 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 curious, is there a particular motivation for doing this now?

(The rustup project isn't doing great on institutional memory right now, so sorry if I'm asking dumb questions, just trying to make sure we have -- and document -- sufficient state for this going forward.)

build.rs Outdated

let target_os = env::var("CARGO_CFG_TARGET_OS");
let target_env = env::var("CARGO_CFG_TARGET_ENV");
if Ok("windows") == target_os.as_deref() && Ok("msvc") == target_env.as_deref() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: please switch the constants with the run-time values, which seems more idiomatic. I'd also like to the invert the condition here to do an early return for non-Windows-MSVC targets.

Next, please add some comments here similar to the PR description to describe what problem we're trying to solve here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean something like:

if target_os.as_deref() != Ok("windows") && target_env.as_deref() != Ok("msvc") {
    return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup!

build.rs Outdated

// # Turn linker warnings into errors
//
// Rust hides linker warnings meaning mistakes may go unnoticed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there generally good reasons to hide these warnings "upstream" (I guess in rustc)? Or is there a good reason that rustup's behavior, in particular, should be different from the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally I think rustc should not be hiding linker warnings but I'm not trying to litigate that here. However, the reason I think it's especially important in this case is that we're directly and explicitly setting some linker arguments and it would not be good if problems with them (e.g. typos) were being silently ignored.

build.rs Outdated
// # Delay load
//
// Delay load dlls that are not "known DLLs".
// Known DLLs are always loaded from the system directory whereas other DLLs
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have some reference documentation links for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Best is https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#factors-that-affect-searching which mentions how it affects search order and where to find the list of known DLLs. Unfortunately the list itself is not documented.

/// rustup-init in the user's download folder.
#[cfg(windows)]
pub fn pre_rustup_main_init() {
use winapi::um::libloaderapi::{SetDefaultDllDirectories, LOAD_LIBRARY_SEARCH_SYSTEM32};
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe orthogonal, but should we start using windows-sys here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm surprised there's not an issue for that already but I do think that's beyond the scope of this PR.

use winapi::um::libloaderapi::{SetDefaultDllDirectories, LOAD_LIBRARY_SEARCH_SYSTEM32};
// Default to loading delay loaded DLLs from the system directory.
unsafe {
let result = SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32);
Copy link
Contributor

Choose a reason for hiding this comment

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

So the Rust 1.72 blog post had this text:

In a future release we're planning to increase the minimum supported Windows version to 10. The accepted proposal in compiler MCP 651 is that Rust 1.75 will be the last to officially support Windows 7, 8, and 8.1. When Rust 1.76 is released in February 2024, only Windows 10 and later will be supported as tier-1 targets. This change will apply both as a host compiler and as a compilation target.

How do you think those changes (5 months from now) should interact with this code?

@ChrisDenton
Copy link
Member Author

Ok, I've updated the comments based on the review. To avoid repeating myself, I only added a short comment to src/bin/rustup-init.rs but referenced build.rs for further info.

for dll in delay_load_dlls {
println!("cargo:rustc-link-arg-bin=rustup-init=/delayload:{dll}.dll");
}
println!("cargo:rustc-link-arg-bin=rustup-init=delayimp.lib");
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT delayimp.lib is not mentioned on the linked page. Do you have a reference for this/why this is necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, see the documentation for the delayload linker option. I've added a comment that links to it.

@ChrisDenton
Copy link
Member Author

I'm not sure why some checks are failing after adding a comment. The failures look entirely unrelated in any case.

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@djc
Copy link
Contributor

djc commented Oct 4, 2023

It looks like the failures happen because we run clippy from beta? It's a little hard to tell because the CI setup is pretty complicated...

@djc
Copy link
Contributor

djc commented Oct 5, 2023

The CI issues should be fixed after merging #3497, would you mind rebasing so we can get a clean CI run?

build.rs Outdated Show resolved Hide resolved
@ChrisDenton
Copy link
Member Author

Btw, sorry for the noisy force pushes, I committed the change on github then accidentally overwrote the github state with my local state so had to manually reapply 😳

@rami3l rami3l merged commit be2c44c into rust-lang:master Oct 5, 2023
16 checks passed
@rami3l rami3l mentioned this pull request Jan 29, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants