Improve symbol visibility handling for dynamic libraries. #38117

Merged
merged 9 commits into from Dec 6, 2016

Projects

None yet

8 participants

@michaelwoerister
Contributor

This will hopefully fix issue #37530 and maybe also #32887.

I'm relying on @m4b to post some numbers on how awesome the improvement for cdylibs is :)

cc @rust-lang/compiler @rust-lang/tools @cuviper @froydnj
r? @alexcrichton

@alexcrichton

Looks great to me!

Could you also post a small tl;dr; of the high-level changes here? I got lost a bit in the refactorings, but it seemed like it was (a) a tweak to how we call the linker and (b) a tweak to sometimes set symbols to hidden. Clarifications on these two though would be welcome :)

Also, I'm curious about the rlib-into-dylib case as well. Presumably rlibs don't have hidden symbols if they're public exported, right? In that case, if it's linked into a cdylib we're using the "fancy linker argument" to not export the symbols? If it's linked into an executable or goes into a staticlib the symbol will still be exported? (unsure on this last bit)

@@ -0,0 +1,26 @@
+include ../tools.mk
+
+all:
@alexcrichton
alexcrichton Dec 1, 2016 Member

This'll probably need a guard or two for only running on Linux (I think).

Either that or it'll need some tweaks to run on OSX/Windows

src/librustc_trans/back/symbol_export.rs
+ config::CrateTypeExecutable |
+ config::CrateTypeStaticlib |
+ config::CrateTypeCdylib => SymbolExportLevel::C,
+ config::CrateTypeProcMacro |
@alexcrichton
alexcrichton Dec 1, 2016 Member

I think we can actually switch this to a c export level, it's essentially like an executable where nothing else will ever link to it again.

@michaelwoerister
michaelwoerister Dec 2, 2016 Contributor

So it is more or less like a cdylib, right? Or a Rust dylib?

@alexcrichton
alexcrichton Dec 2, 2016 Member

Yeah, more like a cdylib

src/librustc_trans/back/lto.rs
- _ => {
- sess.fatal("lto can only be run for executables and \
+ if !crate_type_allows_lto(*crate_type) {
+ sess.fatal("lto can only be run for executables and \
@nagisa
nagisa Dec 1, 2016 Contributor

This message does not match the crate_type_allows_lto function above anymore.

@michaelwoerister
michaelwoerister Dec 2, 2016 Contributor

Will fix it.

@michaelwoerister
Contributor

The main changes in here are:

  1. Every symbol that is not exported is declared as hidden in LLVM in the internalize_symbols pass. Whether a symbol is exported is determined the following way:

    1. Determine the "export threshold" of this compilation session. If we are only compiling cdylibs or staticlibs or executables, this threshold is export just #[no_mangle] extern "non-Rust". If there is a Rust dylib in the mix, the threshold is export anything that is monomorphic, non-inline, and publicly reachable
    2. Walk all declarations and definitions from this crate, internalize if possible. Failing that, mark as hidden if below the threshold.
  2. The new ExportedSymbols data structure computes and stores a SymbolExportLevel for each symbol and makes this available to the code that generates the export list for a given linker invocation. This way, we can generate a tighter export list for a cdylib even though we could not declare things as hidden in the internalize_symbols pass.

  3. Use --version-script instead of --retain-symbols when invoking the linker. This just seems to be some kind of random bug that would lead to our export lists always being ignored by the linker.

I'm just thinking, maybe it would be sufficient to just always rely on explicit export lists and don't bother with marking things as hidden in LLVM. I'm not sure though if LLVM can already do some optimizations based on the fact that something is hidden.

@froydnj
Contributor
froydnj commented Dec 2, 2016

I'm just thinking, maybe it would be sufficient to just always rely on explicit export lists and don't bother with marking things as hidden in LLVM. I'm not sure though if LLVM can already do some optimizations based on the fact that something is hidden.

More information for the compiler is almost always better. I think the roughly equivalent situation in C/C++ on Linux is using --version-script at link time for a shared library rather than passing -fvisibility=hidden and explicitly specifying in the source code the visibility of symbols that you want to export: you'll wind up with the same exported symbols in both cases, but the compiler can generate better code if it knows that you'll never export a given symbol, since you don't have to go through the PLT.

@alexcrichton
Member

Yeah I agree with @froydnj that getting info in the compiler is best. We also want to help out the staticlib case as much as possible where we don't always control the linker invocation.

@michaelwoerister do you have thoughts on the "rlib in dylib case" above

@michaelwoerister
Contributor

I tried to list the various cases in the test.

To summarize:

  • A Rust dylib will export everything from any rlibs it includes
  • A cdylib will only export public, non-Rust, #[no_mangle] symbols from any rlibs it includes.

So this is pretty much what you said, I think.

As for static libs: They inherit whatever we mark things as in LLVM, I'm not sure actually if the version script will cause the linker to put things not in there into the local symbol table, even if not building a dynamic library. Ideally we'd only want things in the staticlib's global symbol table that would also be exported from a cdylib, right?

@alexcrichton
Member

Yeah in theory a staticlib exported symbol list is precisely the same as a cdylib. Unfortunately though, as you've seen, I don't think we can actually do that. Maybe one day we can emit something that's like "pass these extra args to the linker" but we're not quite there just yet.

@michaelwoerister
Contributor

This and what @froydnj mentioned would be good reasons for switching to rlibs that don't contain any machine code. Then we could control all of this at the leaf products by just declaring things as hidden in LLVM.

@cuviper
Contributor
cuviper commented Dec 2, 2016

A Rust dylib will export everything from any rlibs it includes

This is a conservative policy, but not necessarily optimal, right? e.g. A dylib which was linked with the std rlib probably doesn't need to export everything from std.

@michaelwoerister
Contributor

A dylib which was linked with the std rlib probably doesn't need to export everything from std.

Everything that is publicly reachable, not marked with #[inline] and not generic. But yeah, that's still a lot. But it's also what our rules currently say.

Rust dylibs are strange things anyway. I'm wondering if we should get rid of them, or at least declare them as unstable, not that we have cdylibs.

@alexcrichton
Member

switching to rlibs that don't contain any machine code

Hey if we get a fast enough code generator I'm all for this, it'd solve quite a few problems.

@cuviper
Contributor
cuviper commented Dec 2, 2016

For instance, if my library was a FancyVec, then I'd expect it to export std stuff reachable through the Vec interface I'm wrapping. But ideally it would not export anything from unrelated parts of std.

@michaelwoerister
Contributor

@cuviper If FancyVec was part of a cdylib, then nothing at all from std would be re-exported. But since FancyVec is probably a generic type, there is no benefit in putting it into a dynamic library at all. If you put it into an rlib, then you don't have to worry about symbol visibility.

@cuviper
Contributor
cuviper commented Dec 2, 2016

@michaelwoerister There still could be some benefit to a dylib, if there's some part of that fancy algorithm that doesn't depend on the generic type, perhaps only dealing with the length.

But OK, the example doesn't have to be generic, so let's try something concrete. Consider num::BigUint, which wraps a Vec<u32>. Compiled as a dylib, I get a lot of dynamic symbols which are clearly unrelated, like stuff in std::net.

I'm not suggesting this needs to block the PR, just that there's further room for improvement. In practice people should favor rlibs anyway, since there's no stable ABI in a dylib.

@michaelwoerister
Contributor

@cuviper But how does the compiler know what is related and what isn't? Maybe you included some rlib only because you want to make it's symbols available in your dylib? But I agree that this is not exactly a desirable state of affairs.

@alexcrichton
Member

@cuviper note that symbol visibility was a primary reason cdylibs were created. If you want strict control over symbols then dylib isn't the crate type you want (due to the way rustc uses it). A cdylib, however, is intended to be as conservative as possible with symbol exports.

@cuviper
Contributor
cuviper commented Dec 2, 2016

@michaelwoerister

Maybe you included some rlib only because you want to make it's symbols available in your dylib?

Could be, but I think that's a niche case. Maybe there could be a #[link] or something on the crate import to explicitly support that.

@alexcrichton

If you want strict control over symbols then dylib isn't the crate type you want

OK, but I'm saying the "uncontrolled" state of symbols is not ideal. I think the default should only export things the crate itself makes directly reachable. I might want num in a dylib (because reasons), but it's silly for that dylib to carry code for std::net at all, let alone export it.

Maybe we should move this to its own issue. P-low because dylibs are a bit silly anyway.

@vadimcn
Contributor
vadimcn commented Dec 2, 2016

How important is it to allow symbol preemption in dylibs? This feature does not exist on Windows and I can't say I ever missed it.
Could we indeed just mark all symbols as "hidden" or "private" in LLVM IR and use linker scripts to export only the ones we need?

@michaelwoerister
Contributor

Could we indeed just mark all symbols as "hidden" or "private" in LLVM IR and use linker scripts to export only the ones we need?

Does anybody how that would interact with codegen? If LLVM relies on the symbol not being exported but then the linker exports it anyway, that sounds like a potential problem.

@nagisa
Contributor
nagisa commented Dec 2, 2016

LLVM will outright remove code for provably unreachable stuff.

@cuviper
Contributor
cuviper commented Dec 2, 2016 edited

Some platforms have differences in calling convention too -- e.g. IIRC ppc64le can have multiple entry points depending on whether you need to setup the TOC pointer.

@vadimcn
Contributor
vadimcn commented Dec 2, 2016 edited

Does anybody how that would interact with codegen? If LLVM relies on the symbol not being exported but then the linker exports it anyway, that sounds like a potential problem.

If I am reading this correctly, hidden symbols are still available for static linking, however they are not put into the dynamic symbol table, should that object be linked into a shared lib. Also, internal references to them inside the shared lib will not use indirection via PLT.

@michaelwoerister
Contributor

LLVM will outright remove code for provably unreachable stuff.

I don't think that hidden things become unreachable, they are just not in the dynamic symbol table (for dylibs) / go into the local symbol table instead of the global one (for object files/static libs).

Some platforms have differences in calling convention too -- e.g. IIRC ppc64le can have multiple entry points depending one whether you need to setup the TOC pointer.

That sounds like a problem.

@alexcrichton
Member

@michaelwoerister

btw the only changes wrt this PR itself I see before r+ are:

  • The symbol-visibility test is likely to fail on non-Linux
  • The proc-macro crate type can be more restrictive

(just to summarize)

@michaelwoerister
Contributor

The symbol-visibility test is likely to fail on non-Linux

This should work now for OSX and MSVC. On MINGW I see too many symbols included for cdylibs, so there's still a bug in there somewhere.

The proc-macro crate type can be more restrictive

This should be fixed by 581c292 now.

@bors
Contributor
bors commented Dec 3, 2016

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

@alexcrichton
Member

r=me whenever this passes tests, or also feel free to use bors to see if it passes tests :)

@michaelwoerister
Contributor

I disabled tests on MINGW now. The linker there seems to ignore --version-script and --dynamic-symbols. It does something when --retain-symbols is passed, that is, afterwards nm will only list the symbols specified. But dumpbin, which I think is more authoritative on Windows, still lists all symbols under /EXPORTS (@retep998, any clue what that is about?). So on MINGW you get the old behavior basically.

@alexcrichton
Member

@michaelwoerister sounds ok to me, we can always look more into it if it becomes a problem.

@m4b
Contributor
m4b commented Dec 5, 2016

Have you tried --export-all-symbols in combination with the version script?

You might also try messing with --ouput-def and then using it as input to the linker (on mingw windows)

nm is probably printing symbol information from the COFF portion of the binary and not the Microsoft PE extended part, so yea I'd default to ms binutils in this case

@michaelwoerister
Contributor

@bors r=alexcrichton

Let's give it a try :)

@bors
Contributor
bors commented Dec 5, 2016

📌 Commit 8ecdc4e has been approved by alexcrichton

@bors
Contributor
bors commented Dec 5, 2016

⌛️ Testing commit 8ecdc4e with merge daf8c1d...

@bors bors added a commit that referenced this pull request Dec 5, 2016
@bors bors Auto merge of #38117 - michaelwoerister:hidden-symbols, r=alexcrichton
Improve symbol visibility handling for dynamic libraries.

This will hopefully fix issue #37530 and maybe also #32887.

I'm relying on @m4b to post some numbers on how awesome the improvement for cdylibs is `:)`

cc @rust-lang/compiler @rust-lang/tools @cuviper @froydnj
r? @alexcrichton
daf8c1d
@bors bors merged commit 8ecdc4e into rust-lang:master Dec 6, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment