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

MSVC: Avoid using jmp stubs for dll function imports #84758

Merged
merged 2 commits into from
May 23, 2021

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Apr 30, 2021

Windows import libraries contain two symbols for every function: __imp_FunctionName and FunctionName (where FunctionName is the name of the function to be imported).

__imp_FunctionName contains the address of the imported function. This will be filled in by the Windows executable loader at runtime. FunctionName contains a jmp stub that simply jumps to the address given by __imp_FunctionName. E.g. it's a function that solely contains a single jmp instruction:

jmp __imp_FunctionName

When using an external DLL function in Rust, by default the linker will link to FunctionName, causing a bit of indirection at runtime. In Microsoft's C++ it's possible to instead tell it to insert calls to the address in __imp_FunctionName by using the __declspec(dllimport) attribute. In Rust it's possible to get effectively the same behaviour using the #[link] attribute on extern blocks.


The second commit also merges multiple extern blocks into one block. This is because otherwise Rust will currently create duplicate linker arguments for each block. In this case having duplicates shouldn't matter much other than the noise when displaying the linker command.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 Apr 30, 2021
@dtolnay
Copy link
Member

dtolnay commented May 4, 2021

Second commit looks good to me -- thank you.

Could I get a sanity check from some Windows pros on the reasoning in #84758 (comment) and the content of f66c678353407015a8f795f2683f7688759b2b72?

@rustbot ping windows

@rustbot
Copy link
Collaborator

rustbot commented May 4, 2021

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @arlosi @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @rylev @sivadeilra @wesleywiser

@rustbot rustbot added the O-windows Operating system: Windows label May 4, 2021
@nico-abram
Copy link
Contributor

The extern block includes BCryptGenRandom, which according to msdn is in Bcrypt.dll, not kernel32 (Like the link attribute says). Is that correct?

https://github.com/rust-lang/rust/blob/d35d94355d967a076aae85810f43f99b613127b0/library/std/src/sys/windows/c.rs#L716-L717

@ChrisDenton
Copy link
Member Author

ChrisDenton commented May 5, 2021

The extern block includes BCryptGenRandom, which according to msdn is in Bcrypt.dll, not kernel32 (Like the link attribute says). Is that correct?

This is true. However, Rust's #[link] attribute doesn't currently affect how functions in the associated extern block are found. I'm just using it for the side-effect of triggering the dllimport heuristic.

That said, it might be worth separating them properly for the sake of documentation and future proofing.

@sivadeilra
Copy link

The extern block includes BCryptGenRandom, which according to msdn is in Bcrypt.dll, not kernel32 (Like the link attribute says). Is that correct?

This is true. However, Rust's #[link] attribute doesn't currently affect how functions in the associated extern block are found. I'm just using it for the side-effect of triggering the dllimport heuristic.

That said, it might be worth separating them properly for the sake of documentation and future proofing.

It would help to avoid confusion here, by using the correct DLL name. My team is working on implementing the raw-dylib support for Windows, so that the #[link] attribute's specification of the DLL name will be used to generate import tables.

This avoids using jmp stubs when calling functions exported from a dll.
@ChrisDenton
Copy link
Member Author

Ok, I've changed the second commit to use the proper import libraries. Note that #84794 somewhat mitigates the previous issue with duplicate import names, so long as the duplicate #[link] attributes are on consecutive extern blocks. I've taken advantage of that now.

At some point during copy/pasting parts around rustfmt got hold of it, which makes the diff a bit noisier than I'd like. I can try to separate out the formatting changes if that's a problem.

@ChrisDenton
Copy link
Member Author

Btw, in case it helps anyone test my working, here's the powershell commands I'm using to verify the difference this PR makes:

Z:\dllimport> @'
>> fn main() {
>>     println!("Hello World");
>> }
>> '@ > dllimport.rs
Z:\dllimport> rustc dllimport.rs
Z:\dllimport> dumpbin /disasm dllimport.exe | Select-String WriteConsoleW

  000000014000E395: E8 69 D3 00 00     call        WriteConsoleW
  000000014000E3F6: E8 08 D3 00 00     call        WriteConsoleW
WriteConsoleW:
  000000014001B703: FF 25 2F 09 00 00  jmp         qword ptr [__imp_WriteConsoleW]

Z:\dllimport> rustc +dllimport dllimport.rs
Z:\dllimport> dumpbin /disasm dllimport.exe | Select-String WriteConsoleW

  00000001400027BF: FF 15 93 88 01 00  call        qword ptr [__imp_WriteConsoleW]
  0000000140002855: FF 15 FD 87 01 00  call        qword ptr [__imp_WriteConsoleW]

I'm using the "Developer Powershell for VS 2019" but only so dumpbin is in the PATH. The rustc +dllimport is used for a custom rustup toolchain which points to a build with this PR applied.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay
Copy link
Member

dtolnay commented May 22, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 22, 2021

📌 Commit fc40aa0 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2021
Comment on lines -1020 to -1021
// >= Vista / Server 2008
// https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinkw
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why u deleted this comments (and lower)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that only Windows 7+ is supported the detailed version info seems redundant.

bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2021
Rollup of 6 pull requests

Successful merges:

 - rust-lang#84758 (MSVC: Avoid using jmp stubs for dll function imports)
 - rust-lang#85288 (add an example to explain std::io::Read::read returning 0 in some cases)
 - rust-lang#85334 (Add doc aliases to `unit`)
 - rust-lang#85525 (Fix my mailmap entry)
 - rust-lang#85571 (Remove surplus prepend LinkedList fn)
 - rust-lang#85575 (Fix auto-hide for implementations and implementors.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d5fa533 into rust-lang:master May 23, 2021
@bors
Copy link
Contributor

bors commented May 23, 2021

⌛ Testing commit fc40aa0 with merge 92418ce...

@rustbot rustbot added this to the 1.54.0 milestone May 23, 2021
@ChrisDenton ChrisDenton deleted the dllimport branch May 23, 2021 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants