Link std statically in rustc_driver by Zoxc · Pull Request #122362 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation132 Commits6 Checks6 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
This makes rustc_driver statically link to std. This is done by not passing -C prefer-dynamic when building rustc_driver. However building rustc-main won't work currently as it tries to dynamically link to both rustc_driver and std resulting in a crate graph with std duplicated. To fix that new command line option -Z prefer_deps_of_dynamic is added which prevents linking to a dylib if there's a static variant of it already statically linked into another dylib dependency.
The main motivation for this change is to enable #[global_allocator] to be used in rustc_driver allowing overriding the allocator used in rustc on all platforms.
Instead of adding -Z prefer_deps_of_dynamic, this PR is changed to crate opt-in to the linking change via the rustc_private feature instead, as that would be typically needed to link to rustc_driver anyway.
try-job: aarch64-apple
try-job: x86_64-msvc
try-job: i686-mingw
try-job: dist-x86_64-msvc
try-job: aarch64-gnu
r? @oli-obk
rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
This comment has been minimized.
enable
#[global_allocator]to be used inrustc_driverallowing overriding the allocator used in rustc on all platforms.
why can't this be done in rustc-main?
Will this PR require all custom drivers (including our tools) to use -Z prefer_deps_of_dynamic?
does this mean that external tools using rustc driver now also need to use this flag? would be nice if the duplicate std error message mentioned that.
it would be even nicer if this flag could be made the default.. but I'm not sure whether that would be a breaking change
This comment has been minimized.
why can't this be done in
rustc-main?
The global allocator must be statically linked with std.
Will this PR require all custom drivers (including our tools) to use
-Z prefer_deps_of_dynamic?
I made the rustc_private feature imply -Z prefer_deps_of_dynamic, which should avoid that.
This could probably use a perf run as there could be some ThinLTO interactions.
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Link std statically in rustc_driver
This makes rustc_driver statically link to std. This is done by not passing -C prefer-dynamic when building rustc_driver. However building rustc-main won't work currently as it tries to dynamically link to both rustc_driver and std resulting in a crate graph with std duplicated. To fix that new command line option -Z prefer_deps_of_dynamic is added which prevents linking to a dylib if there's a static variant of it already statically linked into another dylib dependency.
The main motivation for this change is to enable #[global_allocator] to be used in rustc_driver allowing overriding the allocator used in rustc on all platforms.
☀️ Try build successful - checks-actions
Build commit: 380a85b (380a85bad098d06e75548877d9061302a9dd5312)
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Use mimalloc as global_allocator for rustc_driver
This changes the Rust global allocator for rustc_driver to mimalloc, leaving LLVM unaffected.
Based on rust-lang#122362.
Finished benchmarking commit (380a85b): comparison URL.
Overall result: ✅ improvements - no action needed
Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.
@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression
Instruction count
This is a highly reliable metric that was used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -0.9% | [-1.7%, -0.4%] | 178 |
| Improvements ✅ (secondary) | -0.9% | [-1.7%, -0.3%] | 164 |
| All ❌✅ (primary) | -0.9% | [-1.7%, -0.4%] | 178 |
Max RSS (memory usage)
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -2.7% | [-2.7%, -2.7%] | 1 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | -2.7% | [-2.7%, -2.7%] | 1 |
Cycles
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 17.9% | [17.8%, 18.0%] | 2 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -1.1% | [-1.2%, -1.0%] | 2 |
| All ❌✅ (primary) | - | - | 0 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 672.448s -> 671.639s (-0.12%)
Artifact size: 310.05 MiB -> 298.66 MiB (-3.67%)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of all this interspersed logic would it be possible to at the end iterate over all dylibs and mark all crates they have linked into them as IncludedFromDylib?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if that's correct. The current logic starts by picking the dylibs then adding other crates as needed. That seems less likely to end up with missing crates than removing them after.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is not to remove crates, but to change their linkage to IncludeFromDylib after it has determined which crates to include.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I mean by remove as IncludeFromDylib won't be linked to the crate. It also won't work with the current duplicate crate detection as that happens as the crates are added.
| @@ -1810,6 +1810,8 @@ options! { |
|---|
| "use a more precise version of drop elaboration for matches on enums (default: yes). \ |
| This results in better codegen, but has caused miscompilations on some tier 2 platforms. \ |
| See #77382 and #74551."), |
| prefer_deps_of_dynamic: bool = (false, parse_bool, [TRACKED], |
| "prefer linking to static dependencies of dynamic libraries over available dynamic libraries (default: no)"), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works robustly, I feel like this should be unconditionally enabled.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I'm not confident in there being no regressions. We don't really have many dylib and multi crate type tests.
| let can_be_rustc_dep = filename.starts_with("rustc_driver-") |
|---|
| | |
| | |
| if can_be_rustc_dep |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this extra logic needed for?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's to prevent the std dynamic library from also being included with the compiler, as it's no longer needed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like it'd be worth a comment in the code. (That's probably true most times a reviewer had a question that was answered by a clarification without code change.)
| @@ -2028,10 +2028,16 @@ impl<'a> Builder<'a> { |
|---|
| // When we build Rust dylibs they're all intended for intermediate |
| // usage, so make sure we pass the -Cprefer-dynamic flag instead of |
| // linking all deps statically into the dylib. |
| if matches!(mode, Mode::Std | Mode::Rustc) { |
| if matches!(mode, Mode::Std) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you will need to add Mode::Codegen here as -Cprefer-dynamic is no longer implied when linking librustc_driver.so
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how there's any change to -C prefer-dynamic for other crates?
The job i686-mingw failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
---- [run-make] tests\run-make\dump-ice-to-disk stdout ----
error: rmake recipe failed to complete
status: exit code: 101
command: "C:\\a\\rust\\rust\\build\\i686-pc-windows-gnu\\test\\run-make\\dump-ice-to-disk\\rmake.exe"
--- stderr -------------------------------
thread 'main' panicked at C:\a\rust\rust\tests\run-make\dump-ice-to-disk\rmake.rs:63:5:
assertion `left == right` failed
left: 60
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Finished benchmarking commit (9cb1998): comparison URL.
Overall result: ✅ improvements - no action needed
@rustbot label: -perf-regression
Instruction count
This is a highly reliable metric that was used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -1.0% | [-2.0%, -0.2%] | 229 |
| Improvements ✅ (secondary) | -1.0% | [-2.0%, -0.2%] | 222 |
| All ❌✅ (primary) | -1.0% | [-2.0%, -0.2%] | 229 |
Max RSS (memory usage)
Results (primary 1.8%, secondary 1.8%)
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | 1.8% | [1.0%, 2.5%] | 2 |
| Regressions ❌ (secondary) | 2.9% | [1.5%, 6.0%] | 4 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -2.5% | [-2.5%, -2.5%] | 1 |
| All ❌✅ (primary) | 1.8% | [1.0%, 2.5%] | 2 |
Cycles
Results (primary -1.1%, secondary -2.4%)
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -1.1% | [-1.8%, -0.8%] | 15 |
| Improvements ✅ (secondary) | -2.4% | [-3.3%, -1.7%] | 8 |
| All ❌✅ (primary) | -1.1% | [-1.8%, -0.8%] | 15 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 758.668s -> 756.29s (-0.31%)
Artifact size: 339.27 MiB -> 341.47 MiB (0.65%)
hlisdero added a commit to hlisdero/cargo-check-deadlock that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
RalfJung pushed a commit to RalfJung/miri that referenced this pull request
Reviewers
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Relevant to the compiler team, which will review and decide on the PR/issue.