Improve symbol visibility handling for dynamic libraries. #38117

michaelwoerister · 2016-12-01T20:15:42Z

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)

alexcrichton · 2016-12-01T21:32:14Z

src/test/run-make/symbol-visibility/Makefile

@@ -0,0 +1,26 @@
+include ../tools.mk
+
+all:


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

alexcrichton · 2016-12-01T21:43:47Z

src/librustc_trans/back/symbol_export.rs

+        config::CrateTypeExecutable |
+        config::CrateTypeStaticlib  |
+        config::CrateTypeCdylib     => SymbolExportLevel::C,
+        config::CrateTypeProcMacro  |


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.

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

Yeah, more like a cdylib

nagisa · 2016-12-01T23:18:06Z

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 \


This message does not match the crate_type_allows_lto function above anymore.

Will fix it.

michaelwoerister · 2016-12-02T16:01:34Z

The main changes in here are:

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.
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.
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 · 2016-12-02T16:20:49Z

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 · 2016-12-02T17:18:58Z

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 · 2016-12-02T17:34:57Z

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 · 2016-12-02T17:36:41Z

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 · 2016-12-02T17:41:45Z

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 · 2016-12-02T17:42:11Z

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 · 2016-12-02T17:47:20Z

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 · 2016-12-02T17:50:09Z

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 · 2016-12-02T17:50:59Z

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 · 2016-12-02T18:22:53Z

@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 · 2016-12-02T18:47:13Z

@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 · 2016-12-02T18:55:03Z

@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 · 2016-12-02T19:03:06Z

@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 · 2016-12-02T19:17:43Z

@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 · 2016-12-02T19:29:30Z

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 · 2016-12-02T19:34:10Z

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 · 2016-12-02T19:35:39Z

LLVM will outright remove code for provably unreachable stuff.

cuviper · 2016-12-02T19:38:56Z

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 · 2016-12-02T19:42:29Z

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 · 2016-12-02T20:17:32Z

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 · 2016-12-02T22:40:26Z

@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 · 2016-12-03T01:47:45Z

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 · 2016-12-03T07:54:24Z

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

alexcrichton · 2016-12-03T20:04:00Z

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

michaelwoerister · 2016-12-05T16:12:13Z

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 · 2016-12-05T16:39:07Z

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

m4b · 2016-12-05T19:03:50Z

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 · 2016-12-05T19:20:57Z

@bors r=alexcrichton

Let's give it a try :)

bors · 2016-12-05T19:20:58Z

📌 Commit 8ecdc4e has been approved by alexcrichton

bors · 2016-12-05T20:48:07Z

⌛ Testing commit 8ecdc4e with merge daf8c1d...

@m4b

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

bors · 2016-12-06T00:12:19Z

rust nightly after Dec-8 will have a problem with crates generating cdylib with cargo clippy (tested with clippy version 0.0.104) - it fails at linking step with - syntax error in version script - error. This is apparantly a bug due to rust-lang/rust#38117. So reverting back to using rust-nightly of 17th Nov 2016 and clippy of v0.0.99.

rust-highfive assigned alexcrichton Dec 1, 2016

alexcrichton reviewed Dec 1, 2016

View reviewed changes

nagisa reviewed Dec 1, 2016

View reviewed changes

michaelwoerister force-pushed the hidden-symbols branch 2 times, most recently from 6077ba8 to 4c675a9 Compare December 3, 2016 01:44

michaelwoerister force-pushed the hidden-symbols branch from 4c675a9 to 2863736 Compare December 5, 2016 15:56

michaelwoerister added 9 commits December 5, 2016 11:05

Make LLVM symbol visibility FFI types more stable.

d1a6d47

trans: Rename reachable to exported_symbols where appropriate.

5fd7c2b

Refactor symbol export list generation.

133aeac

Generate a version script for linkers on Linux.

e48160f

Linking: Include export lists in debug output.

bfd4910

Add test case for symbol visibility in dylibs.

ea9f613

Export the plugin registrar from proc-macro crates (and not much else)

93da4f8

Mention cdylibs in LTO error message.

3548c8f

Make symbol-visibility test case work on all platforms.

8ecdc4e

michaelwoerister force-pushed the hidden-symbols branch from 2863736 to 8ecdc4e Compare December 5, 2016 16:05

bors merged commit 8ecdc4e into rust-lang:master Dec 6, 2016

m4b mentioned this pull request Dec 8, 2016

export_name with unusual utf8 breaks new version script based linker #38238

Open

japaric mentioned this pull request Dec 10, 2016

rust_begin_unwind required for panic=abort #38281

Closed

alexcrichton mentioned this pull request Jan 24, 2017

Static linking without LTO on Linux does not remove unused code #39270

Closed

retep998 mentioned this pull request Feb 1, 2017

cargo rustc -- -Zno-trans called on crate_type = ["cdylib"] crates abort linker #39445

Closed

Mark-Simulacrum mentioned this pull request May 15, 2017

don't make intra-crate calls to exported functions go through the PLT or similar #32887

Closed

eddyb mentioned this pull request Jul 29, 2022

Proc macros should limit symbol exports #99909

Closed

Improve symbol visibility handling for dynamic libraries. #38117

Improve symbol visibility handling for dynamic libraries. #38117

Conversation

michaelwoerister commented Dec 1, 2016

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Dec 1, 2016

Choose a reason for hiding this comment

alexcrichton Dec 1, 2016

Choose a reason for hiding this comment

michaelwoerister Dec 2, 2016

Choose a reason for hiding this comment

michaelwoerister Dec 2, 2016

Choose a reason for hiding this comment

alexcrichton Dec 2, 2016

Choose a reason for hiding this comment

nagisa Dec 1, 2016

Choose a reason for hiding this comment

michaelwoerister Dec 2, 2016

Choose a reason for hiding this comment

michaelwoerister commented Dec 2, 2016

froydnj commented Dec 2, 2016

alexcrichton commented Dec 2, 2016

michaelwoerister commented Dec 2, 2016

alexcrichton commented Dec 2, 2016

michaelwoerister commented Dec 2, 2016

cuviper commented Dec 2, 2016

michaelwoerister commented Dec 2, 2016

alexcrichton commented Dec 2, 2016

cuviper commented Dec 2, 2016

michaelwoerister commented Dec 2, 2016

cuviper commented Dec 2, 2016

michaelwoerister commented Dec 2, 2016

alexcrichton commented Dec 2, 2016

cuviper commented Dec 2, 2016

vadimcn commented Dec 2, 2016

michaelwoerister commented Dec 2, 2016

nagisa commented Dec 2, 2016

cuviper commented Dec 2, 2016 • edited Loading

vadimcn commented Dec 2, 2016 • edited Loading

michaelwoerister commented Dec 2, 2016

alexcrichton commented Dec 2, 2016

michaelwoerister commented Dec 3, 2016

bors commented Dec 3, 2016

alexcrichton commented Dec 3, 2016

michaelwoerister commented Dec 5, 2016

alexcrichton commented Dec 5, 2016

m4b commented Dec 5, 2016

michaelwoerister commented Dec 5, 2016

bors commented Dec 5, 2016

bors commented Dec 5, 2016

bors commented Dec 6, 2016

cuviper commented Dec 2, 2016 •

edited

Loading

vadimcn commented Dec 2, 2016 •

edited

Loading