Skip to content

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

Merged
bors merged 3 commits into
rust-lang:masterfrom
vadimcn:nobundle
Feb 4, 2017
Merged

Implement kind="static-nobundle" (RFC 1717)#38426
bors merged 3 commits into
rust-lang:masterfrom
vadimcn:nobundle

Conversation

@vadimcn

@vadimcn vadimcn commented Dec 17, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

r? @pnkfelix

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

@vadimcn

vadimcn commented Dec 17, 2016

Copy link
Copy Markdown
Contributor Author

r? @alexcrichton

cc @retep998

@retep998

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

vadimcn commented Dec 21, 2016

Copy link
Copy Markdown
Contributor Author

yah, no worries!

@bors

bors commented Dec 21, 2016

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Dec 30, 2016

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Jan 12, 2017

Copy link
Copy Markdown
Collaborator

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

@vadimcn

vadimcn commented Jan 19, 2017

Copy link
Copy Markdown
Contributor Author

rebased

@Jascha-N

Copy link
Copy Markdown
Contributor

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

bors commented Jan 19, 2017

Copy link
Copy Markdown
Collaborator

☔ 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

vadimcn commented Jan 25, 2017

Copy link
Copy Markdown
Contributor Author

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

@alexcrichton

Copy link
Copy Markdown
Member

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

@alexcrichton

Copy link
Copy Markdown
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

vadimcn commented Jan 27, 2017

Copy link
Copy Markdown
Contributor Author

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

retep998 commented Jan 27, 2017

Copy link
Copy Markdown
Contributor

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

vadimcn commented Jan 27, 2017

Copy link
Copy Markdown
Contributor Author

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

@retep998

retep998 commented Jan 27, 2017

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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

retep998 commented Jan 27, 2017

Copy link
Copy Markdown
Contributor

@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

vadimcn commented Jan 27, 2017

Copy link
Copy Markdown
Contributor Author

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

@alexcrichton

Copy link
Copy Markdown
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

vadimcn commented Jan 30, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

@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
Copy Markdown
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

vadimcn commented Feb 2, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

vadimcn commented Feb 3, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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?

Don't link "nobundle" libs which had  already been included in upstream crate.
@vadimcn

vadimcn commented Feb 4, 2017

Copy link
Copy Markdown
Contributor Author

Okay, I think this is it!

@alexcrichton

Copy link
Copy Markdown
Member

@bors: r+

Thanks @vadimcn!

@bors

bors commented Feb 4, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 7504897 has been approved by alexcrichton

@bors

bors commented Feb 4, 2017

Copy link
Copy Markdown
Collaborator

⌛ 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

bors commented Feb 4, 2017

Copy link
Copy Markdown
Collaborator

☀️ 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