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

Implement kind="static-nobundle" (RFC 1717) #38426

Merged
merged 3 commits into from
Feb 4, 2017
Merged

Conversation

vadimcn
Copy link
Contributor

@vadimcn vadimcn commented Dec 17, 2016

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.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@vadimcn
Copy link
Contributor Author

vadimcn commented Dec 17, 2016

r? @alexcrichton

cc @retep998

@retep998
Copy link
Member

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.

@alexcrichton
Copy link
Member

@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.

@vadimcn
Copy link
Contributor Author

vadimcn commented Dec 21, 2016

yah, no worries!

@bors
Copy link
Contributor

bors commented Dec 21, 2016

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

@bors
Copy link
Contributor

bors commented Dec 30, 2016

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

@bors
Copy link
Contributor

bors commented Jan 12, 2017

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

@vadimcn
Copy link
Contributor Author

vadimcn commented Jan 19, 2017

rebased

@Jascha-N
Copy link

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.

@bors
Copy link
Contributor

bors commented Jan 19, 2017

☔ 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.
@vadimcn
Copy link
Contributor Author

vadimcn commented Jan 25, 2017

@alexcrichton: ping, this has been sitting in the queue for a while.

@alexcrichton
Copy link
Member

@vadimcn gah sorry for taking so long to get to this, taking a look now

@alexcrichton
Copy link
Member

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.

@vadimcn
Copy link
Contributor Author

vadimcn commented Jan 27, 2017

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?

@retep998
Copy link
Member

retep998 commented Jan 27, 2017

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.

@vadimcn
Copy link
Contributor Author

vadimcn commented Jan 27, 2017

@retep998: uh, yes, that's what I meant. Not the final-final, but "final in the chain of .rlibs".

@retep998
Copy link
Member

retep998 commented Jan 27, 2017

@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.

@alexcrichton
Copy link
Member

@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?

@retep998
Copy link
Member

retep998 commented Jan 27, 2017

@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.

@vadimcn
Copy link
Contributor Author

vadimcn commented Jan 27, 2017

@alexcrichton: my intention was that the native lib would only get linked to B.

@alexcrichton
Copy link
Member

@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?

@vadimcn
Copy link
Contributor Author

vadimcn commented Jan 30, 2017

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.

@retep998
Copy link
Member

@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.

@alexcrichton
Copy link
Member

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

@vadimcn
Copy link
Contributor Author

vadimcn commented Feb 2, 2017

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?

@alexcrichton
Copy link
Member

@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.

@vadimcn
Copy link
Contributor Author

vadimcn commented Feb 3, 2017

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.

@alexcrichton
Copy link
Member

Hm I forget precisely where that happens but it should be I think roughly the same code path somewhere that kind = "static" takes, right?

@vadimcn
Copy link
Contributor Author

vadimcn commented Feb 4, 2017

Okay, I think this is it!

@alexcrichton
Copy link
Member

@bors: r+

Thanks @vadimcn!

@bors
Copy link
Contributor

bors commented Feb 4, 2017

📌 Commit 7504897 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 4, 2017

⌛ Testing commit 7504897 with merge ea7a648...

bors added a commit that referenced this pull request Feb 4, 2017
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.
@bors
Copy link
Contributor

bors commented Feb 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ea7a648 to master...

@bors bors merged commit 7504897 into rust-lang:master Feb 4, 2017
@vadimcn vadimcn deleted the nobundle branch February 6, 2017 19:46
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.

7 participants