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

Use #[link(kind)] to fix imports from native libs on Windows #1717

Merged
merged 2 commits into from
Oct 25, 2016

Conversation

vadimcn
Copy link
Contributor

@vadimcn vadimcn commented Aug 16, 2016

Make compiler aware of the association between library names adorning extern blocks and symbols defined within the block. Add attributes and command line switches that leverage this association.

(based on RFC1296, minus the dllexport bits)

Rendered

@ahicks92
Copy link

Two questions/problems:

  1. Is this needed for dlls for which I have the corresponding .lib? My understanding is that it isn't because the .lib contains the necessary code to do what Rustc would generate, but my understanding of how this actually works when you have the .lib from the C++ perspective is "cool, magic black box".
  2. It is incredibly useful in some cases to link to dlls for which no import library is available. Does this RFC allow for that? An example of this is wanting to link a library that only compiles with MSVC to MinGW or vice versa: you can, but it's easier for everyone involved if you just promise that the function has this signature and comes from a dll with whatever name.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 16, 2016

Is this needed for dlls for which I have the corresponding .lib? My understanding is that it isn't because the .lib contains the necessary code to do what Rustc would generate, but my understanding of how this actually works when you have the .lib from the C++ perspective is "cool, magic black box".

It's still needed. The dllimport attribute alters code generation on the caller side.

It is incredibly useful in some cases to link to dlls for which no import library is available. Does this RFC allow for that? An example of this is wanting to link a library that only compiles with MSVC to MinGW or vice versa: you can, but it's easier for everyone involved if you just promise that the function has this signature and comes from a dll with whatever name.

No, link.exe does not support linking to a .dll without an import library. You can create import library from a dll with either msvc tools or mingw tools.

@retep998
Copy link
Member

It is incredibly useful in some cases to link to dlls for which no import library is available. Does this RFC allow for that? An example of this is wanting to link a library that only compiles with MSVC to MinGW or vice versa: you can, but it's easier for everyone involved if you just promise that the function has this signature and comes from a dll with whatever name.

No, link.exe does not support linking to a .dll without an import library. You can create import library from a dll with either msvc tools or mingw tools.

Note that it is possible to have rustc emit idata sections and avoid the need for import libraries, but that's not part of the scope of this RFC. If someone wants to create an RFC for rustc emitting idata, go for it. rust-lang/rust#30027

@Ericson2314
Copy link
Contributor

Ericson2314 commented Aug 16, 2016

This is great! I'm not even primarily windows user, but I was thinking about how originally rustc was very anti endless CLI options, but then Cargo came along any many of those decisions were no longer the purview of the rust source. This is the best of both worlds in the source code can ensure it gets what it needs, but not mandate exactly how that's accomplished.

On to the details, is it possible to just skip the kind = ... in the annotation rather than use kind = "abstract"? Or does that already mean something else? I think this is plain cleaner, and avoids leading people to believe -l abstract=... works on the CLI---a clear victory. Ah, I see "dylib" is already the implicit default.

@ahicks92
Copy link

I'm using slightly poor terminology, for which I apologize. I'm aware that link.exe doesn't take dlls.

I would love to see another RFC for this. The flexibility of just being able to import without caring is a feature I really wish more native languages had. Unfortunately, I don't know enough about LLVM to even try (but perhaps that is unnecessary).

@nrc nrc added the T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. label Aug 16, 2016
@alexcrichton
Copy link
Member

cc @nikomatsakis, you had many opinions on #1296 as well.

Otherwise looks great to me, thanks @vadimcn!

@eternaleye
Copy link

eternaleye commented Aug 24, 2016

I'd argue that "abstract" is superfluous, as discussed in the "unresolved questions" - in particular, the ability to override non-kind=abstract linkages, combined with the absence of a kind meaning dylib, means that leaving it off works exactly the same as putting kind=abstract when an overriding kind is specified.

As a further benefit, this means that the approach most people will default to (i.e. the least typing) is the one the tooling expects for such cases, allowing them to fall into the "correctness hole."

Another possibility, to address the "drawbacks" section, is that an additional tool could be written to validate that the #[link] attribute is well-formed even on non-Windows systems - this could even be gradually made more stringent, first by Cargo linting on publish, then linting on build, then enforcing on publish.

@retep998
Copy link
Member

@eternaleye How can you validate whether the user used dylib or static-nobundle correctly on Windows? All Rust sees is that you're linking to a .lib. Whether a library is effectively being linked statically or dynamically can differ between platforms as well, especially when it is the user who is responsible for providing the library so who knows whether it will be static or dynamic. Really the best we can do here is take advantage of appveyor CI where possible and hope that Windows users file issues with the relevant crate when they run into linking problems.

@eternaleye
Copy link

eternaleye commented Aug 24, 2016

@retep998: That's actually almost exactly backwards of both where and what I was thinking of validating. In particular, I was thinking of validating that the declared symbols are present in (and maybe only in, as more lenient checks have differing behavior across OSes) the declared library.

As this check is possible on any OS, it could catch some classes of error before Windows CI is ever run.

@retep998
Copy link
Member

@eternaleye That still seems rather complicated and brittle as Rust/Cargo needs to have access to the library to analyze it and determine whether the symbols exist (which is hard when the library is being pulled in as a system dependency so only the linker knows where it actually is), nevermind that would only verify the current configuration, it wouldn't know whether the declared library on other platforms has those symbols too. Furthermore verifying that the symbols are only in that library would mean analyzing every single other library to ensure they don't have the symbols.

Really I think this would be better suited by the crate just having tests to ensure example binaries link correctly and running it on CI on a variety of platforms.

@nikomatsakis
Copy link
Contributor

@vadimcn

One clarification. In the RFC, you have this example:

#[link(name = "baz", kind="abstract")]
extern {
// dllimport not applied, "baz" not linked
}

If the user uses the command line to specify that baz is coming from a dylib, we would apply dllimport in that case, right? That is, it'd be equivalent to kind="dylib"?

@nikomatsakis
Copy link
Contributor

@eternaleye

I'd argue that "abstract" is superfluous

I agree that it is not needed. It seems cleaner to me, though, and doesn't seem like a very complex addition. If you expect the details of this library to be specified in the Cargo.toml, why not say so? It makes it clearer to the reader of the source code what is happening (this varies between platforms and configurations; consult the Cargo.toml). But I won't fight all that hard.

Another way of looking at it is that it is roughly like the difference between doing:

let x = if foo { 22 } else { 23 };

versus:

let mut x = 23;
if foo { x = 22; }

combined with the absence of a kind meaning dylib

I don't follow this clause. There is literally a kind dylib shown in the RFC example, so what does the "absence of a kind meaning dylib" mean?

@nikomatsakis
Copy link
Contributor

This part of the RFC feels ... underspecified to me:

For this use case we'll introduce another library "kind", "static-nobundle". Such libraries would be treated in the same way as "static", minus the bundling.

But maybe that's all that needs to be said? I'm not close enough to the code to say for sure I guess.

@eternaleye
Copy link

@nikomatsakis

#[link(name = "foo")] // defaults to kind = "dylib"
extern {
    // ...
}

See #1717 (comment)

As this "dylib" can be overridden, the convention can then be "leave it off for Cargo" - and there could also be lints if a defaulted kind is not overridden, if we decide to go that way.

@Ericson2314
Copy link
Contributor

Hmm, that's not a bad idea: add a warning now, and maybe for Rust 2.0 change its meaning so specifying a kind on the CLI is mandatory. Leaving out a kind is definitely the more intuitive syntax than kind="abstract", but we don't want people to be bit with forgetting to override either.

@retep998
Copy link
Member

retep998 commented Aug 24, 2016

This part of the RFC feels ... underspecified to me:

For this use case we'll introduce another library "kind", "static-nobundle". Such libraries would be treated in the same way as "static", minus the bundling.

But maybe that's all that needs to be said? I'm not close enough to the code to say for sure I guess.

Well, you'd have to know when to pass it to the linker, which is particularly important when you are creating Rust dylibs (cdylibs don't count here). For kind=dylib you can just pass it to all downstream linker invocations. For kind=static-nobundle however you'd only pass it to the first linker invocation, aka the one where the crate that declared those externs is being linked as an .rlib (or even if the crate itself is a dylib/exe). From then on out any further downstream linker invocations could only be using that crate as a Rust dylib and so the extern library wouldn't be passed to the linker there because otherwise you'd end up with two sources of truth for something like a static variable which is bad. However, due to the nature of Rust inlining and monomorphization (or even simply publicly exporting those symbols in your crate), a downstream crate that goes over a dylib boundary might still end up needing to link to the symbols from the external library so that dylib in which the external library was linked into would have to re-export those symbols. This would act similar to the way the current unstable #[linked_from] works, such as for example librustc_llvm which is usually a dylib and has to re-export stuff from LLVM so other rustc crates can use them. That said, this re-exporting behavior is not unique to kind=static-nobundle, as kind=static would have to deal with it too. The only part unique to kind=static-nobundle is just knowing when to pass it to the linker.

But yes, all of this should be obvious to anyone familiar enough with this sort of stuff, although it wouldn't hurt to clarify it in the RFC.

@nikomatsakis
Copy link
Contributor

@eternaleye

As this "dylib" can be overridden, the convention can then be "leave it off for Cargo" - and there could also be lints if a defaulted kind is not overridden, if we decide to go that way.

I guess it's a matter of taste. I don't consider this a win, for the reasons I already gave. It's ambiguous whether I actually wanted dylib or not. It seems like the result would be "less good" error messages when I fail to override: instead of saying "please supply some information about where to find the abstraction dependency vector-math", it would say "couldn't find dylib vector-math", which implies both that (a) vector-math is the right name (it may not be) and (b) we want a dylib (we may not).

But anyway it's a minor point.

@eternaleye
Copy link

eternaleye commented Aug 24, 2016

@nikomatsakis: But with a warning, you would get a good error message.

Warning: "kind" was not specified on the #[link] attribute for "vector-math",
         but no override was provided. Undeclared "kind" defaulting to "dylib"
         is deprecated; please specify an override on the command line.
Error: couldn't find dylib "vector-math"

@Ericson2314
Copy link
Contributor

Ericson2314 commented Aug 24, 2016

@nikomatsakis

Even better, avoid the word "override" to make clear how things ought to be:

Warning: no linking kind is specified in the #[link] attribute for "vector-math",
         but no kind was provided on the command line either. The defaulting of
         undeclared kinds to "dylib" is deprecated; please specify a kind on the 
         command line.
Error: couldn't find dylib "vector-math"

And:

Warning: a linking kind was specified both in the #[link] attribute for "vector-math"
         and on the command line. The command line's kind will override the attribute's.

@nikomatsakis
Copy link
Contributor

OK, I yield, I yield! :) I like the idea of "no kind without an override" being explicitly deprecated, and defaulting just for backwards compat, as opposed to just being defined as falling back to dylib.

@alexcrichton
Copy link
Member

@rfcbot fcp merge

We've had a lot of discussion on this over time, lots of discussion in amongst the tools team historically, so I think this is ready for merging!

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 5, 2016

Team member alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.
Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@michaelwoerister
Copy link
Member

@rfcbot reviewed

@vadimcn
Copy link
Contributor Author

vadimcn commented Oct 11, 2016

@rfcbot reviewed

@Ericson2314
Copy link
Contributor

Can we get "abstract" removed as per @eternaleye and me above before merging?

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 17, 2016

All relevant subteam members have reviewed. No concerns remain.

@alexcrichton
Copy link
Member

🔔 This RFC is now entering its final comment period for merging 🔔

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Oct 17, 2016
@Ericson2314
Copy link
Contributor

3rd above #1717 (comment) :)

@alexcrichton
Copy link
Member

@vadimcn, thoughts?

@vadimcn
Copy link
Contributor Author

vadimcn commented Oct 18, 2016

Yeah, I'm fine with dropping kind="abstract".
However, the warning proposed here doesn't make sense to me: omitting kind="..." is fine on other platforms, since 'dylib' is the most frequently desired linkage, but on WIndows it's 'deprecated'?

@eternaleye
Copy link

eternaleye commented Oct 18, 2016

@vadimcn: The dylib kind isn't what's deprecated; what's deprecated is assuming it as the default if it is unspecified in the source, without being given as part of the build process.

Basically: An undeclared kind used to mean dylib, and going forward it should mean "set by the buildsystem." This helps avoid silent platform compatibility failures.

@vadimcn
Copy link
Contributor Author

vadimcn commented Oct 18, 2016

The dylib kind isn't what's deprecated; what's deprecated is assuming it as the default if it is unspecified in the source, without being given as part of the build process.

Yes, I realize that.
So you want to make it deprecated on every platform for the sake of a corner case on Windows? Wouldn't this warning will be emitted for most every *-sys crate?
IMO, this isn't worth it.

Reword "minus unbundling".
@Ericson2314
Copy link
Contributor

Ericson2314 commented Oct 18, 2016

@vadimcn I think most sys crates should support dynamic or static linking, and this reminds them to do that or be explicit they don't. And because of mutable globals and what not, static vs dynamic isn't transparent on unix either.

So I think this change RFC is necessary for Windows, but good medicine for all platforms :).

@retep998 retep998 mentioned this pull request Oct 19, 2016
47 tasks
@rfcbot
Copy link
Collaborator

rfcbot commented Oct 24, 2016

It has been one week since all blocks to the FCP were resolved.

@alexcrichton
Copy link
Member

Ok, sounds like the brought up concerns were addressed, and otherwise sounds like we're on board! Merging, thanks again for the RFC @vadimcn!

Tracking issue: rust-lang/rust#37403

@alexcrichton alexcrichton merged commit 2ebe33c into rust-lang:master Oct 25, 2016
@Centril Centril added A-linkage Proposals relating to the linking step. A-attributes Proposals relating to attributes labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Proposals relating to attributes A-linkage Proposals relating to the linking step. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-dev-tools Relevant to the development tools team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.