rustc: Tweak visibility of some lang items #52993
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Note that the diff looks gnarly, but the second commit is much cleaner and the first commit should have no functional changes included, just a refactoring. |
This was referenced Aug 2, 2018
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
alexcrichton
force-pushed
the
fix-some-vis
branch
from
fb9f0f0
to
236adc0
Compare
|
The travis failure above pointed me to some more worrying bugs to fix, but they should be fixed here now as well. The bug was that we were defining the We are, today, generating an allocator CGU for too many crates. If you link to the standard library dynamically, for example, you don't need an allocator CGU as libstd has already selected the allocator symbols for you, and you should reference those. This meant that the compiler failed to compile because it was generating allocator CGUs referencing hidden symbols defined in libstd's dynamic object, causing the link to fail. So the solution here was to generate an allocator CGU less of the time. Basically if anything is a rust dylib there's no need for an allocator CGU because the allocator has already been selected. Otherwise everything should proceed as usual today (all static compilations will continue to generate an allocator CGU) |
| tcx: TyCtxt<'a, 'tcx, 'tcx>, | ||
| mono_item: &MonoItem<'tcx>, | ||
| can_be_internalized: &mut bool, | ||
| default: &dyn Fn(DefId, bool) -> Visibility, |
There was a problem hiding this comment.
It seems like default could be just a regular function and doesn't need to be passed in as a closure.
| // available to downstream crates. This depends on whether we are in | ||
| // share-generics mode and whether the current crate can even have | ||
| // downstream crates. | ||
| let export_generics = tcx.share_generics() && |
There was a problem hiding this comment.
It might be good to compute this value just once (because doesn't depend on the specific mono-item) and pass it in as a simple boolean parameter.
| // FIXME: eventually we don't want to always force this symbol to have | ||
| // hidden visibility, it should indeed be a candidate for | ||
| // internalization, but we have to understand that it's referenced | ||
| // from the `main` symbol we'll generate later. |
There was a problem hiding this comment.
This might be solvable by have a special InstanceDef variant, like we have for all the shims...
There was a problem hiding this comment.
(Just thinking aloud, no action needed here)
The previous iteration was a large `match` which was quite heavily indented, making it slightly difficult to read and see what was going on. This iteration is a refactoring (no functional change intended) to make it a bit easier on the eyes and a bit easier to modify over time.
alexcrichton
force-pushed
the
fix-some-vis
branch
from
236adc0
to
4afcb44
Compare
|
@bors: r=michaelwoerister |
|
📌 Commit 4afcb44eb00575386162136f6ed715e8659240f9 has been approved by |
bors
added
S-waiting-on-bors
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors: r- |
bors
added
S-waiting-on-author
This commit tweaks the linker-level visibility of some lang items that rustc uses and defines. Notably this means that `#[panic_implementation]` and `#[alloc_error_handler]` functions are never marked as `internal`. It's up to the linker to eliminate these, not rustc. Additionally `#[global_allocator]` generated symbols are no longer forced to `Default` visibility (fully exported), but rather they're relaxed to `Hidden` visibility). This symbols are *not* needed across DLL boundaries, only as a local implementation detail of the compiler-injected allocator symbols, so `Hidden` should suffice. Closes rust-lang#51342 Closes rust-lang#52795
alexcrichton
force-pushed
the
fix-some-vis
branch
from
4afcb44
to
7c58ab6
Compare
|
@bors: r=michaelwoerister |
|
📌 Commit 7c58ab6 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 8, 2018
rustc: Tweak visibility of some lang items This commit tweaks the linker-level visibility of some lang items that rustc uses and defines. Notably this means that `#[panic_implementation]` and `#[alloc_error_handler]` functions are never marked as `internal`. It's up to the linker to eliminate these, not rustc. Additionally `#[global_allocator]` generated symbols are no longer forced to `Default` visibility (fully exported), but rather they're relaxed to `Hidden` visibility). This symbols are *not* needed across DLL boundaries, only as a local implementation detail of the compiler-injected allocator symbols, so `Hidden` should suffice. Closes #51342 Closes #52795
|
☀️ Test successful - status-appveyor, status-travis |
|
FWIW, this PR improved compile speed a bit on a couple of benchmarks, and regressed it on |
|
Intersting! I have a hunch for where the slowdown came from, I'll try to test it out. |
alexcrichton
added a commit
to alexcrichton/rust
that referenced
this pull request
Aug 19, 2018
This is an attempt to recover a perf loss observed in rust-lang#52993 by making the result of the `codegen_fn_attrs` query cheap to clone as more and more parts of the compiler start to use this query.
bors
added a commit
that referenced
this pull request
Aug 19, 2018
Make `codegen_fn_attrs` query cheap to clone This is an attempt to recover a perf loss observed in #52993 by making the result of the `codegen_fn_attrs` query cheap to clone as more and more parts of the compiler start to use this query.
|
My attempt in #53499 to diagnose this unfortunately was unsuccessful |
|
This injected a bug that caused My current guess is that @alexcrichton was incorrect that " |
|
(BTW its certainly posslble that the correct fix here lies with |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit tweaks the linker-level visibility of some lang items that rustc
uses and defines. Notably this means that
#[panic_implementation]and#[alloc_error_handler]functions are never marked asinternal. It's up tothe linker to eliminate these, not rustc.
Additionally
#[global_allocator]generated symbols are no longer forced toDefaultvisibility (fully exported), but rather they're relaxed toHiddenvisibility). This symbols are not needed across DLL boundaries, only as a
local implementation detail of the compiler-injected allocator symbols, so
Hiddenshould suffice.Closes #51342
Closes #52795