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

Declare Windows imports jobserver depends on #106

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

ChrisDenton
Copy link
Member

Jobserver is specifying external functions without also declaring a dependency on the relevant import library that contains those functions. While kernel32 will often be linked by something, the same can not be said for advapi32. As a bonus this is mildly more efficient.

In the future we could use raw-dylib to avoid the need for import libs entirely (requires msrv to be 1.65+),

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the patch!

As a bonus this is mildly more efficient.

Out of curiosity, how does it get more efficient with this change?

Copy link
Member Author

@ChrisDenton ChrisDenton Sep 17, 2024

Choose a reason for hiding this comment

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

I wrote a bit about something similar on zulip: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Link.20attribute.20on.20Windows/near/470532686

It's a super minor thing and almost certainly doesn't affect the performance of jobserver. In short, an extern block defaults to static linkage whereas #[link] defaults to dynamic linkage. Since this function is from a DLL we want dynamic linkage because the location of a function is stored at an address in the import table rather than a static function you can call directly.

Static linkage wouldn't work at all except import libraries add a "jump stub". That is a kinda of dummy function that just jumps to the real function. It's the equivalent to this:

fn CloseHandle(handle: HANDLE) -> BOOL {
    (*__imp_CloseHandle)(handle)
}

So there's an extra level of indirection. Like, I said it's super minor and only rarely matters.

@weihanglo weihanglo merged commit adc71a5 into rust-lang:main Sep 17, 2024
15 checks passed
@ChrisDenton ChrisDenton deleted the imports branch September 17, 2024 21:56
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.

2 participants