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

Inform the msvc linker about local and upstream native libraries #84264

Closed
wants to merge 1 commit into from

Conversation

ChrisDenton
Copy link
Member

This gives downstream crates the ability to override upstream native libraries.

In the MSVC linker there are two ways to specify a library:

  • as a standalone argument on the command line
  • using /DEFAULTLIB:

The first way cannot be overridden, except by a library that appears earlier on the command line. There is no way to remove such a library. On the other hand, using /DEFAULTLIB: allows overriding or removing a library using /NODEFAULTLIB:. Thus using /DEFAULTLIB: for upstream native libraries grants more control over linking. This is especially useful for overriding standard library imports which isn't as easy to patch as third party crates. It is also consistent with the behaviour of Visual C++'s #pragma comment(lib, "libname"). Such consistency is useful in mixed Rust/C++ code bases.

This PR does effectively de-prioritise upstream libraries but crate local libraries were always favoured.

This gives downstream crates the ability to override upstream native libraries
@rust-highfive
Copy link
Collaborator

r? @varkor

(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 16, 2021
@petrochenkov
Copy link
Contributor

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned varkor Apr 17, 2021
@petrochenkov
Copy link
Contributor

Still didn't get to reviewing this in detail, but I'm generally skeptical about introducing more linker-specific behavior.

We have more generic Rust mechanisms for doing things like you've mentioned.
-Z link-native-libraries=no gives an effect similar to /NODEFAULTLIB and "library renaming" on command line -l name:rename allows to override libraries linked via #[link(name = "...")] attributes.

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Apr 21, 2021

-Z link-native-libraries=no gives an effect similar to /NODEFAULTLIB

I'm having trouble finding the documentation for this. Can it be used to disable only a single library?

-l name:rename allows to override libraries linked via #[link(name = "...")] attributes.

That is indeed an option. Though it is awkward to override dependencies.


In any case, my goal here is not for Rust to introduce more linker specific behaviour. I simply think that Rust linking behaviour should be implemented in a similar way to MSVC C++ libraries when using the MSVC toolchain (if it makes sense to do so).

So for this Rust code:

#[link(name="library")]
extern {}

The MSVC C++ equivalent is:

#pragma comment(lib, "library")

I think they should try to behave the same way, so long as it still fits Rust's semantics. This is especially useful in a mixed Rust/C++ code base. Especially so that standard tooling can be used.

@ChrisDenton
Copy link
Member Author

I think the short version is that I'm not trying to change or add features to Rust. I'm trying to get Rust to integrate better with MSVC tooling.

@petrochenkov
Copy link
Contributor

petrochenkov commented May 4, 2021

#pragma comment(lib, "mylib") doesn't make distinction between "local and upstream" libraries, it always emits /DEFAULTLIB:mylib, so the analogy is broken here.
I also feel skeptical about introducing subtle and platform-dependent differences between libraries linked from the local crate and dependencies, such differences didn't previously exist.

(That said, we don't necessarily have to keep such analogy. Yes, #[link] is a way to link libraries from source code, but it's a pretty general functionality, it shouldn't necessarily follow what #pragma comment(lib, "library") specifically does.)

Though it is awkward to override dependencies.

Adding -l foo:bar to rustc flags is no more awkward than adding -Clink-arg=/NODEFAULTLIB:foo -l bar.
Do you have any specific use case that you want to address?

I'm having trouble finding the documentation for this. Can it be used to disable only a single library?

No, -Z link-native-libraries=no is a pretty heavy hammer.

I think what we should do is to support an empty override in -l name:override like -l foo:, with the meaning of removing the library from linking rather than overriding it with another library (the syntax is similar to branch deleting in git).

(Right now you can override a library into nothing with some dirty workaround like -l foo:my_empty_lib or -l foo:libcmt.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2021
@bors
Copy link
Contributor

bors commented May 6, 2021

☔ The latest upstream changes (presumably #84982) made this pull request unmergeable. Please resolve the merge conflicts.

@ChrisDenton
Copy link
Member Author

Ok, I need to think about this some more. Here's a powershell session that I think highlights one of the issues I'm trying to resolve. It creates a library and a binary, both with native imports, then tries to override the native libraries when building the binary.

Z:\native> @'
>> #![crate_type="rlib"]
>> #[link(name="upstream")] extern {}
>> '@ > lib.rs
Z:\native> @'
>> #[link(name="library")] extern {}
>> fn main() {}
>> '@ > main.rs
Z:\native> rustc +nightly lib.rs
Z:\native> rustc +nightly main.rs --extern lib=liblib.rlib -l library:msvcrt -l upstream:msvcrt --edition 2018
error: renaming of the library `upstream` was specified, however this crate contains no `#[link(...)]` attributes referencing this library.

error: aborting due to previous error

The other issue (which you've touched on) is that I can also remove any native library so long as Rust doesn't explicitly import it. For example, kernel32.lib is (currently) implicitly imported via msvcrt.lib. Becuase msvcrt.lib uses /DEFAULTLIB:, I am able to tell the linker to disable this import (which also allows me to replace it). Whereas if a Rust library imports a library then I lose this ability.

The advantage of using /DEFAULTLIB: is that it would allow using the same tools across ecosystems. Maybe a C or C++ library links the native lib or maybe a Rust one does. Either way I would be able to override them at the link stage and in the same way.

@petrochenkov
Copy link
Contributor

renaming of the library upstream was specified, however this crate contains no #[link(...)] attributes referencing this library.

Hmm, I assumed this is a bug, but looks like the original RFC (https://github.com/rust-lang/rfcs/blob/master/text/1717-dllimport.md#library-name-and-kind-variance) tells that overrides indeed work only for the current crate.
This is certainly pretty limiting, it means you cannot e.g. link to debug CRT (#39016) using this method.

I'd like to try making -l overrides work for transitively linked libraries in some form, so we have an overriding method that works on all platforms rather than just on MSVC.
I plan to audit the library linking code in May/June (to move #81490 to stabilization, fix #73319, etc) so I'd prefer to defer the decision about switching to /DEFAULTLIB: until that work is done.

@ChrisDenton
Copy link
Member Author

Ok. I'll close this now as any outstanding issues can be revisited once more linking features have been implemented.

@ChrisDenton ChrisDenton closed this May 8, 2021
@ChrisDenton ChrisDenton deleted the defaultlib branch May 8, 2021 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants