Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link std statically in rustc_driver #122362

Merged
merged 6 commits into from
Aug 11, 2024
Merged

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 12, 2024

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

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2024

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 12, 2024
@rust-log-analyzer

This comment has been minimized.

@Zoxc Zoxc force-pushed the rustc_driver_static_std branch from d3ef302 to 9506132 Compare March 12, 2024 05:50
@oli-obk
Copy link
Contributor

oli-obk commented Mar 12, 2024

enable #[global_allocator] to be used in rustc_driver allowing 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?

@Noratrieb
Copy link
Member

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

@rust-log-analyzer

This comment has been minimized.

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 12, 2024

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.

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 12, 2024

This could probably use a perf run as there could be some ThinLTO interactions.

@Kobzol
Copy link
Contributor

Kobzol commented Mar 12, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 12, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2024
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.
@bors
Copy link
Contributor

bors commented Mar 12, 2024

⌛ Trying commit d5cdb9e with merge 380a85b...

@bors
Copy link
Contributor

bors commented Mar 12, 2024

☀️ Try build successful - checks-actions
Build commit: 380a85b (380a85bad098d06e75548877d9061302a9dd5312)

@rust-timer

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2024
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.
@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 12, 2024
}
}
}
}
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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)"),
Copy link
Member

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.

Copy link
Contributor Author

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-")
|| filename.starts_with("librustc_driver-")
|| build_compiler.stage == 0;
if can_be_rustc_dep
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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) {
Copy link
Member

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

Copy link
Contributor Author

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?

@bors bors added S-waiting-on-bors 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 Aug 11, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Aug 11, 2024

This doesn't need to block this PR, but we should probably either ignore or modify these failing tests in the dist builders that run tests. It's wasteful to ship a 12 MiB file in our dist artifacts that is unused.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 11, 2024
…obk,lqd,bjorn3,Kobzol

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.

---

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
@bors
Copy link
Contributor

bors commented Aug 11, 2024

⌛ Testing commit b22253c with merge d5dfd7a...

@rust-log-analyzer
Copy link
Collaborator

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

@bors
Copy link
Contributor

bors commented Aug 11, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review 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 Aug 11, 2024
@jieyouxu
Copy link
Member

Lol. @bors retry

@bors bors added S-waiting-on-bors 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 Aug 11, 2024
@bors
Copy link
Contributor

bors commented Aug 11, 2024

⌛ Testing commit b22253c with merge 9cb1998...

@bors
Copy link
Contributor

bors commented Aug 11, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk,lqd,bjorn3,Kobzol
Pushing 9cb1998 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 11, 2024
@bors bors merged commit 9cb1998 into rust-lang:master Aug 11, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 11, 2024
@lqd
Copy link
Member

lqd commented Aug 11, 2024

🎉

@rust-timer
Copy link
Collaborator

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%)

@Zoxc Zoxc mentioned this pull request Aug 11, 2024
@Zoxc Zoxc deleted the rustc_driver_static_std branch August 11, 2024 20:35
hlisdero added a commit to hlisdero/cargo-check-deadlock that referenced this pull request Aug 30, 2024
Now we need `#![feature(rustc_private)]` in the `main.rs`

See the original PR
rust-lang/rust#122362

And the PR in another tool that requires the same change
trailofbits/dylint#1298

Update lock file too
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2024
…, r=onur-ozkan

Do not copy libstd dynamic library to sysroot

Since rust-lang#122362, rustc links statically to libstd.[so|dll]. Which means that the libstd.[so|dll] file no longer has to be in the rustc sysroot. However, we are currently still shipping this file, in every new release of Rust, for no reason, it's just wasted bytes.

This PR removes the dynamic library file from the built sysroot.

However, it is not yet performed on Windows, because stage0 incremental tests start failing there (see description of the issue [here](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Failing.20incr.20tests.20on.20Windows.20when.20std.2Edll.20is.20missing/near/474507064)).

This is an extended version of rust-lang#128986.
CC `@Zoxc`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Oct 14, 2024
…ozkan

Do not copy libstd dynamic library to sysroot

Since rust-lang/rust#122362, rustc links statically to libstd.[so|dll]. Which means that the libstd.[so|dll] file no longer has to be in the rustc sysroot. However, we are currently still shipping this file, in every new release of Rust, for no reason, it's just wasted bytes.

This PR removes the dynamic library file from the built sysroot.

However, it is not yet performed on Windows, because stage0 incremental tests start failing there (see description of the issue [here](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Failing.20incr.20tests.20on.20Windows.20when.20std.2Edll.20is.20missing/near/474507064)).

This is an extended version of rust-lang/rust#128986.
CC `@Zoxc`
@tamird
Copy link
Contributor

tamird commented Oct 18, 2024

This seems to have broken the compiletest-rs build in a way I'm not sure how to fix. #[feature(rustc_private)] is already present: https://github.com/Manishearth/compiletest-rs/blob/7d4e29ac98a74255bcc49f8d6de9132d20b792c3/src/lib.rs#L12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.