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

Use protected visibility when building rustc #131634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidlattimore
Copy link
Contributor

rust-lang/compiler-team#782

I wasn't sure about having two commits in a PR, but I figured, at least initially it might make sense to discuss these commits together. Happy to squash, or move the second commit to a separate PR.

I contemplated trying to enable protected visibility for more cases when LLD will be used other than just -Zlinker-features=+lld, but that would be more a complex change that probably still wouldn't cover all cases when LLD is used, so went with the simplest option of just checking if the linker-feature is enabled.

r? lqd

@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 Oct 13, 2024
@tgross35
Copy link
Contributor

I wasn't sure about having two commits in a PR, but I figured, at least initially it might make sense to discuss these commits together. Happy to squash, or move the second commit to a separate PR.

Multiple atomic commits in a single PR are totally fine, that's easier to review and better for history anyway (the way you have this PR is ideal imo). We definitely don't want a bunch of back and forth WIP-style commits that make changes just to undo them or fix things up, but there is no 1PR=1commit rule like LLVM.

@nikic
Copy link
Contributor

nikic commented Oct 13, 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 Oct 13, 2024
@bors
Copy link
Contributor

bors commented Oct 13, 2024

⌛ Trying commit 1e1bf2b with merge 6ad0952...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2024
Use protected visibility when LLD feature is enabled and enable it when building rustc

rust-lang/compiler-team#782

I wasn't sure about having two commits in a PR, but I figured, at least initially it might make sense to discuss these commits together. Happy to squash, or move the second commit to a separate PR.

I contemplated trying to enable protected visibility for more cases when LLD will be used other than just `-Zlinker-features=+lld`, but that would be more a complex change that probably still wouldn't cover all cases when LLD is used, so went with the simplest option of just checking if the linker-feature is enabled.

r? lqd
@bors
Copy link
Contributor

bors commented Oct 13, 2024

☀️ Try build successful - checks-actions
Build commit: 6ad0952 (6ad095273a92c251fb0c1697fd745ab6caf21d2c)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6ad0952): 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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.6% [-34.2%, -0.2%] 212
Improvements ✅
(secondary)
-4.6% [-29.9%, -0.2%] 225
All ❌✅ (primary) -1.6% [-34.2%, -0.2%] 212

Max RSS (memory usage)

Results (primary -2.2%, secondary -3.2%)

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.2% [-6.5%, -0.5%] 146
Improvements ✅
(secondary)
-3.2% [-7.0%, -0.6%] 151
All ❌✅ (primary) -2.2% [-6.5%, -0.5%] 146

Cycles

Results (primary -3.3%, secondary -7.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)
-3.3% [-24.2%, -0.6%] 53
Improvements ✅
(secondary)
-7.4% [-22.6%, -1.9%] 94
All ❌✅ (primary) -3.3% [-24.2%, -0.6%] 53

Binary size

Results (primary -0.3%, secondary -0.7%)

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)
-0.3% [-0.8%, -0.1%] 11
Improvements ✅
(secondary)
-0.7% [-0.8%, -0.5%] 37
All ❌✅ (primary) -0.3% [-0.8%, -0.1%] 11

Bootstrap: 783.66s -> 778.727s (-0.63%)
Artifact size: 332.16 MiB -> 331.51 MiB (-0.20%)

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

lqd commented Oct 14, 2024

I was expecting we'd only do this when using lld by default on stable. Just using a different default linker on nightly has bitten us at least once in the recent past, and I didn't think we'd want the 2 configs to drift even more. But if others are fine with it, so am I.

Cool, as expected these results match bjorn's old prototype I mentioned in the MCP.

However, could you explain a bit about the intent? Did you want to change visibility when using lld in general, or when using rust-lld? Because just checking -Zlinker-features=+lld flag is not sufficient to know whether either of these will be true. The linker flavor can be inferred from other arguments or the target, rust-lld needs self-contained linking enabled by the target/CLI (and to use cg_llvm and not other backends).

I wasn't sure about having two commits in a PR

It's very much welcomed in general, however in this case I think we may need to extract the second commit in another PR: the exact handling of use-lld in bootstrap is being reworked by t-bootstrap. Until this clean-up and tests are done, depending on which stage we pass this flag, I'm not personally sure that we couldn't be unknowingly switching from system lld to beta/stage 1 rust-lld, so I'll let @Kobzol comment on this change. If you did this specifically to get protected visibility on our dist artifacts, setting the default visibility explicitly in the CI config seems better to me, but again I'm not on t-infra/t-bootstrap and they will know better.

@Kobzol
Copy link
Contributor

Kobzol commented Oct 14, 2024

Perf. results look great! 🎉 The bootstrap change is a bit drive-by, though. We should finally refactor the way we handle LLD in bootstrap, I'll try to priotize it soon-ish. That's partially orthogonal to this PR, though.

I wonder if we should first enable this only for the compiler itself? Both to separate the perf. effect on the compiler from the perf. effect on the binaries, and also to test it for a while on nightly on the compiler before we enable it by default for all binaries compiled by a nightly compiler.

@davidlattimore
Copy link
Contributor Author

My intent was primarily, at least initially to use protected visibility for rustc-driver. Not tying it to LLD and flagging it on for just rustc initially sounds sensible to me. One thing I'm not sure about though is whether that means I'll need to wait until -Zdefault-visibility hits stable, which, I think would be November 28th. Unless there's a way to only add the flag for later stages - although I can imagine that might not be desirable.

@Kobzol
Copy link
Contributor

Kobzol commented Oct 15, 2024

I would only do it for stage 2, we already do a bunch of optimizations on stage 2 only anyway, e.g. PGO and BOLT.

So when building stage 2 rustc, I'd just forcefully set the -Zdefault-visibility flag.

@lqd
Copy link
Member

lqd commented Oct 15, 2024

One thing I'm not sure about though is whether that means I'll need to wait until -Zdefault-visibility hits stable, which, I think would be November 28th

bootstrap uses beta and RUSTC_BOOTSTRAP so the stable cycle is less important. If you wanted to enable this at stage 1, then it'd only need #130005 to land on beta, which should be in the next couple days. If you want to only enable it at stage 2, you can do it now.

IIRC for stage-dependent flags, it should be done in bootstrap, e.g. around here (similarly to -Zdylib-lto, which is not set at stage 1). Then temporarily comment setting this CI environment variable and we can do a try build with smoke tests enabled.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2024
@@ -428,7 +428,7 @@ pub fn linker_flags(
) -> Vec<String> {
let mut args = vec![];
if !builder.is_lld_direct_linker(target) && builder.config.lld_mode.is_used() {
args.push(String::from("-Clink-arg=-fuse-ld=lld"));
args.push(String::from("-Zlinker-features=+lld"));
Copy link
Member

@bjorn3 bjorn3 Oct 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep building the standard library with default visibility as CI will build with lld, but the end user may then use this version of the standard library with ld.bfd. For librustc_driver.so it is completely fine though. Same for tools and codegen backends.

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 a very good point. I suspect that means that parts of the standard library that make it into the dylib, will unfortunately still have default visibility, but there might not be much we can do about that in the short term - unless we were to build a copy of the standard library specifically for use in rustc_driver that used protected visibility, then distribute a copy of the standard library that used default visibility. I guess that'd be like building the compiler with -Zbuild-std and -Zdefault-visibility=protected.

@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2024

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rust-log-analyzer

This comment has been minimized.

@davidlattimore
Copy link
Contributor Author

As suggested, I've updated this PR so that LLD is no longer involved. I now just set -Zdefault-visibility=protected when building rustc. This means that libstd isn't built with protected symbols, which means we do have a few more relocations than before, so the performance gain might not be as good as in the original check. However, it still should give some benefit. My stage2 rustc now has 1438 GLOB_DAT relocations, whereas the stage0 without this change has 7078. I think the previous way of enabling protected visibility got down to around 700 GLOB_DAT relocations - but that relied on compiling libstd with the flag, which likely would have broken anyone trying to use that libstd with GNU ld < 2.40.

I added a flag --protected-symbol-definitions that defaults to true. This was done in case someone wants to turn off protected symbols when building rustc for some reason. Is this OK having this flag default to true, or perhaps I should instead have a flag that has the opposite meaning and defaults to false? e.g. --default-visibility-definitions.

@mati865
Copy link
Contributor

mati865 commented Oct 31, 2024

Please update PR name.

@davidlattimore davidlattimore changed the title Use protected visibility when LLD feature is enabled and enable it when building rustc Use protected visibility when building rustc Oct 31, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2024

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@davidlattimore
Copy link
Contributor Author

I decided that I didn't like the flag, so I've changed it so that now it only adds a new option to config.toml. Now you can set:

[rust]
use-protected-symbols = true

This new option defaults to true if use-lld is set, so should affect release builds of rustc.

I haven't updated change_tracker.rs, since I don't expect anyone to need to edit their config.toml as a result of this change, but can add an entry it if you think it's needed.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 31, 2024
@lqd
Copy link
Member

lqd commented Oct 31, 2024

As I’m not on t-bootstrap, r? Kobzol

@rustbot rustbot assigned Kobzol and unassigned lqd Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.