-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Implement kind="static-nobundle" (RFC 1717) #38426
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
cc @retep998 |
Not only that, but when working with Rust dylibs, a |
@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. |
yah, no worries! |
☔ 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. |
rebased |
I would like to see this get merged, since my project depends on it. Linking to static symbols (such as |
☔ 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:
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. 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 |
@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...
If two rust dylibs pull in the same rlib (that rlib being the thing that links to that |
@vadimcn the case that I'd specifically be worried about is something like:
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. |
@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 |
Okay, I think this is it! |
📌 Commit 7504897 has been approved by |
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.
☀️ Test successful - status-appveyor, status-travis |
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.