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 sys/compat should use LoadLibrary instead of GetModuleHandle #78444

Closed
m-ou-se opened this issue Oct 27, 2020 · 2 comments
Closed

Windows sys/compat should use LoadLibrary instead of GetModuleHandle #78444

m-ou-se opened this issue Oct 27, 2020 · 2 comments
Labels
O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Oct 27, 2020

From #77618 (comment):

I did however want to comment on the compat_fn macro - I was curious how that was implemented. It uses GetModuleHandleW to find the module to pass to GetProcAddress when it should instead be using LoadLibraryW. Using GetModuleHandleW assumes the module has already been loaded by the calling process. That may be a safe bet for certain functions that you want to “delay load” but certainly not all. LoadLibraryW will just go the extra mile of also loading the module if it has not already been loaded. There's also no need to call FreeLibrary for such APIs so it should be a pretty simple fix.

@sivadeilra
Copy link

Hi, folks. Windows dev, here. Depending on the function and the context, either GetModuleHandle or LoadLibrary is the better choice. They have different constraints.

LoadLibrary cannot be used in certain contexts, such as recursively, within an existing LoadLibrary call. That means it cannot be used in any code path called by a DllMain function, such as responding to a DLL_PROCESS_ATTACH notification. For related reasons, LoadLibrary cannot be used within any module initializer, such as a C++ constructor called for a global variable. So, using LoadLibrary would restrict those places where Rust code could be used.

I ran into exactly this problem while working on integrating Rust code into a Windows DLL.

The best solution is probably to extend Rust so that it produces more than one target for Windows: one for "downlevel" (maximizing compatibility), and one for much more recent builds of Windows, such as Win8 and beyond. That way, we could remove the use of delayed binding to these functions, and use them directly.

I realize creating a new target is way out of scope for just this issue, of course.

@ChrisDenton ChrisDenton added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 12, 2023
@ChrisDenton
Copy link
Member

I agree with sivadeilra that we can't use LoadLibrary in the standard library without restricting how Rust's std can be used. The standard library will not use LoadLibrary going forward.

Ideally we would have a way for users to specify the minimum OS version they support and for the std to adapt accordingly. Cargo's build-std feature may help here but that's some way off. In any case, that's a separate issue.

Closing this now but I'm happy for it to be reopened if you think there needs to be more discussion.

@ChrisDenton ChrisDenton closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants