Implement kind="static-nobundle" (RFC 1717) by vadimcn · Pull Request #38426 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation34 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This implements the "static-nobundle" library kind (last item from #37403).
Rustc handles "static-nobundle" libs very similarly to dylibs, except that on Windows, uses of their symbols do not get marked with "dllimport". Which is the whole point of this feature.
r? @pnkfelix
(rust_highfive has picked a reviewer for you, use r? to override)
except that on Windows, uses of their symbols do not get marked with "dllimport". Which is the whole point of this feature.
Not only that, but when working with Rust dylibs, a static-nobundle library is only linked into the first immediate binary (either dylib or exe) and not later artifacts, in addition to symbols from a static-nobundle library being re-exported from a Rust dylib when necessary. However this doesn't affect 99% of Rust users who just use cargo to build everything normally.
@vadimcn thanks for the PR! I've been a bit overloaded recently but wanted to say that I haven't forgotten this, may just take me a moment to get around to reviewing it.
☔ The latest upstream changes (presumably #38099) made this pull request unmergeable. Please resolve the merge conflicts.
☔ The latest upstream changes (presumably #38697) made this pull request unmergeable. Please resolve the merge conflicts.
☔ The latest upstream changes (presumably #38814) made this pull request unmergeable. Please resolve the merge conflicts.
I would like to see this get merged, since my project depends on it. Linking to static symbols (such as CLSIDs/IIDs) in Windows libraries is quite the hassle at the moment.
☔ The latest upstream changes (presumably #38465) made this pull request unmergeable. Please resolve the merge conflicts.
These are static libraries that are not bundled (as the name implies) into rlibs and staticlibs that rustc generates, and must be present when the final binary artifact is being linked.
@alexcrichton: ping, this has been sitting in the queue for a while.
@vadimcn gah sorry for taking so long to get to this, taking a look now
Ok looks good to me on the surface, but are these the semantics we want? The RFC is pretty light on the details, only mentioning:
For this use case we'll introduce another library "kind", "static-nobundle". Such libraries would be treated in the same way as "static", except they will not be bundled into the target .lib/.rlib.
The logic here seems to be the exact same as dylibs in terms of propagation of the linker flags, right? If so, then that seems like it can quickly run into duplicate symbol errors. If we end up linking a staticlib multiple times then those symbols will be exported from a number of objects.
Just confirming, but those are the semantics we want? I can imagine situations where that does indeed work out such as import libraries on Windows or staticlibs with hidden symbols on Unix, so it's not guaranteed to cause problems per se.
Perhaps "treated in the same way as "static", except they will not be bundled into the target .lib/.rlib." was not the right way to put this.
The semantics I want is: when a .rlib crate depends on a "static-nobundle" library, we remember this fact, but don't include any code from it into the .rlib. This dependency info is propagated all the way to the final downstream dylib or executable, when the lib's name finally gets passed to the linker.
Even if linker ends up with multiple copies of that lib (i.e. ... -lfoo -lfoo -lfoo), this won't cause duplicate symbol errors.
The only case where we might have duplicate symbols is if two Rust dylibs re-exporting the same symbol get used by the same crate further downstream. But this would be detected and reported by the Rust compiler, right?
In my view of how static-nobundle should behave, it isn't passed to the link of the final dylib or executable but rather to the first immediate dylib or executable. If it is a dylib, then if the symbols from the static-nobundle library are externally reachable from that dylib then they are re-exported from that dylib so downstream crates can obtain those symbols from that dylib (similar to what we do with static except we're not necessarily exporting from the same crate that pulled in those symbols originally). If we only pass it to the final downstream dylib or executable, then intermediate dylibs will be unable to link correctly if they depend on any of those symbols because intermediate dylibs do not depend on the final downstream dylib/executable.
@retep998: uh, yes, that's what I meant. Not the final-final, but "final in the chain of .rlibs".
@vadimcn If what I said is indeed what you meant...
The only case where we might have duplicate symbols is if two Rust dylibs re-exporting the same symbol get used by the same crate further downstream. But this would be detected and reported by the Rust compiler, right?
If two rust dylibs pull in the same rlib (that rlib being the thing that links to that static-nobundle library), then you can't link together those two rust dylibs or rustc will complain about that rlib being duplicated. If two different rlibs depend on the same static-nobundle library then ideally they should both have the same links = "foo" specified in their Cargo.toml and cargo will complain. So basically we shouldn't have to worry about duplicate symbols here, at the very least it is no worse than static.
@vadimcn the case that I'd specifically be worried about is something like:
- You've got three crates, A, B, and C
- A depends on B which depends on C
- C has a static-nobundle library
- A, B are dylibs where C is an rlib
This means that the native library can be statically linked into both A and B, duplicating the symbols across those objects. If you then link to A and B I think you could get a duplicate symbol error?
@alexcrichton In that situation the external library from C would be linked only into B because B is the only binary which is statically linking C. A does not statically link C, therefore the external library would not be linked into A. Because B does have the external library linked into it, it would re-export the symbols from the external library as necessary in case A needs them when it depends on B.
@alexcrichton: my intention was that the native lib would only get linked to B.
@vadimcn yes that's my understanding as well, but I believe this PR as-is would have the bad behavior I mentioned which links it to both A and B?
Hrm, yes looks like libfoo is getting passed to A linker as well. This does not cause any problems, though, because there are no unresolved symbols from libfoo left.
@vadimcn I disagree. If B has a generic or inline function, then it isn't actually instantiated in B, so it isn't resolved in B. If A then instantiates that function it'll pull in symbols from the external library, except it'll pull them in from its copy. If the external library has any sort of state, it will be duplicated across A and B resulting in very bad times.
Yeah @retep998 described the scenario I'm worried about. I've basically always considered the same static library on the linker command line multiple times as an inevitable source of bugs, so we should try to avoid that if at all possible
Okay, so B should export all symbols of the native library, that were publicly re-exported from C. This part seems to be working, btw.
The only thing remaining is to make sure that libnative does not get passed to the linker when creating A.
I guess it shouldn't be included in B's metadata? Not quite sure how to do that... @alexcrichton, any hints?
@vadimcn yeah your understanding matches mine at least!
We'll definitely need to encode in the metadata what kind of libraries are being linked, and then for any particular linker invocation we know what format everything is in so we should in theory know whether to pass the linker argument or not. We could walk up the dependency tree and if anything is a dylib we don't pass it and otherwise it's passed.
This version works... except on -msvc toolchain. For whatever reason, symbols reexported from a nobundle lib do not make it to the list of dylib crate exports.
Hm I forget precisely where that happens but it should be I think roughly the same code path somewhere that kind = "static" takes, right?
Don't link "nobundle" libs which had already been included in upstream crate.
Okay, I think this is it!
📌 Commit 7504897 has been approved by alexcrichton
bors added a commit that referenced this pull request
Implement kind="static-nobundle" (RFC 1717)
This implements the "static-nobundle" library kind (last item from #37403).
Rustc handles "static-nobundle" libs very similarly to dylibs, except that on Windows, uses of their symbols do not get marked with "dllimport". Which is the whole point of this feature.