Enable zstd for debug compression. by khuey · Pull Request #125642 · 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
Conversation233 Commits4 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 }})
Set LLVM_ENABLE_ZSTD alongside LLVM_ENABLE_ZLIB so that --compress-debug-sections=zstd is an option.
See #120953
try-job: x86_64-gnu-tools
r? @onur-ozkan
rustbot has assigned @onur-ozkan.
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)
labels
To elaborate a bit here, in #99464 LLVM was updated to version 15, the first to have LLVM_ENABLE_ZSTD. That was explicitly disabled (to avoid pulling in a dependency, according to the PR).
That was fine until #124129 made rust-lld the default linker. Now, because LLVM_ENABLE_ZSTD=OFF disables --compress-debug-sections=zstd, rust-lld is (potentially) a regression against the system linker.
So here I'd like to turn this on so rust-lld and thus the out of the box default is at least as capable as the system linker here.
This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.
How are you producing this zstd-compressed debuginfo without building your own rustc/llvm to add zstd support in -Zdebuginfo-compression?
-C link-arg=-Wl,--compress-debug-sections=zstd
That's what I expected, and it feels suboptimal that rustc/llvm think zstd compression is disabled here: we could disable self-contained linking in that case, or could have emitted a warning/error. I'm sure you're already aware that the linker change is easily controllable without more link args, to fall back to system lld (-Clink-self-contained=-linker -Zunstable-options) or bfd (-Zlinker-features=-lld).
It does emit an error: note: rust-lld: error: --compress-debug-sections: LLVM was not built with LLVM_ENABLE_ZSTD or did not find zstd at build time
I'm aware that I can explicitly opt into the system linker, but this is trivial to fix in rust-lld so I feel we may as well fix it so things just work.
but this is trivial to fix in rust-lld so I feel we may as well fix it so things just work
I'm aware and I agree, I just don't know the rationale in bootstrap yet, so there may be concerns about adding the dependency and we'll have to wait for more info from t-bootstrap / infra.
@nikic was the one who toggled this off. Based on the comments at the time, am I correct in I believing there was no particular reason beyond "let's not accidentally depend on libzstd.so"? Thus it should be fine to do it as long as we are doing it on purpose?
@nikic was the one who toggled this off. Based on the comments at the time, am I correct in I believing there was no particular reason beyond "let's not accidentally depend on libzstd.so"? Thus it should be fine to do it as long as we are doing it on purpose?
I think we still wouldn't want a dependency on libzstd.so, which probably has a non-negligible chance of not being already installed? If we enable zstd we should probably statically link it.
Okay, "we want everything to just be rolled up into our LLVM" sounds fine to me.
Comment on lines 377 to 383
| // Disable zstd to avoid a dependency on libzstd.so. |
|---|
| cfg.define("LLVM_ENABLE_ZSTD", "OFF"); |
| // Libraries for ELF compression. |
| if !target.is_windows() { |
| cfg.define("LLVM_ENABLE_ZLIB", "ON"); |
| cfg.define("LLVM_ENABLE_ZSTD", "ON"); |
| } else { |
| cfg.define("LLVM_ENABLE_ZLIB", "OFF"); |
| cfg.define("LLVM_ENABLE_ZSTD", "OFF"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about making this configurable in config.toml (off by default) instead of constantly setting it as "ON" ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of this PR to enable it by default. If it's not enabled by default it's easier to disable the self-contained linker than to build rustc, including LLVM, from scratch.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why we would make something configurable without a specific reason to do so.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this require an additional dependency to be installed? Also, according to the LLVM docs when it's not set LLVM build enables zstd only if it exists on the system. Could that be an option here?
Fwiw using "FORCE_ON" makes the build raise a hard error when zstd can't be found on the system.
ref: https://llvm.org/docs/CMake.html
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this require an additional dependency to be installed?
Raising the requirements to build our LLVM recipe seems fine given that we support using an external LLVM. Thus we can't assume that the external LLVM has zlib or zstd support (as both are optional) and must query it at runtime (which we do), and we support building an LLVM with arbitrary configuration already: just bring your own.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this require an additional dependency to be installed?
As the patch stands today, it requires libzstd.so to be available at runtime if libzstd was available at build time. It doesn't actually require libzstd to be available at build time (because it uses ON instead of FORCE_ON).
There's a matrix of possible outcomes here.
| ON | FORCE_ON | |
|---|---|---|
| No LLVM_USE_STATIC_ZSTD | Not required at build time, required at runtime if present at build time, silently disabled if not present at build time | Required at build time and runtime |
| LLVM_USE_STATIC_ZSTD | Not required at build time or runtime, silently disabled if not present at build time | Required at build time, not at runtime |
Where we're currently in the top left cell.
If we enable zstd we should probably statically link it.
Once this is done, I am okay with landing it.
Unfortunately statically linking zstd is not currently as simple as setting LLVM_USE_STATIC_ZSTD=True. llvm-config outputs the flags for dynamic linking regardless of the value of LLVM_USE_STATIC_ZSTD.
Relevant to the compiler team, which will review and decide on the PR/issue.
label
Okay, seems to work now. The commit history now contains some reverts though, maybe it wouldn't be such a bad idea to clean it up a little. Could you please compress it down to a smaller number of commits? Thank you.
Set LLVM_ENABLE_ZSTD alongside LLVM_ENABLE_ZLIB so that --compress-debug-sections=zstd is an option. Use static linking to avoid a new runtime dependency. Add an llvm.libzstd bootstrap option for LLVM with zstd. Set it off by default except for the dist builder. Handle llvm-config --system-libs output that contains static libraries.
Build libzstd from source because the EPEL package is built without fPIC.
I reorganized these into something halfway sensible.
Time for yet another try run!
Thanks. Let's try again.
@bors r+
📌 Commit 8db318c has been approved by Kobzol
It is now in the queue for this repository.
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
Enable zstd for debug compression.
Set LLVM_ENABLE_ZSTD alongside LLVM_ENABLE_ZLIB so that --compress-debug-sections=zstd is an option.
See rust-lang#120953
try-job: x86_64-gnu-tools
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:32:5:
assertion `left == right` failed
left: 59
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
That failure is not related to this PR.
@bors retry
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 (68d2e8a): comparison URL.
Overall result: no relevant changes - no action needed
@rustbot label: -perf-regression
Instruction count
This benchmark run did not return any relevant results for this metric.
Max RSS (memory usage)
Results (primary 3.2%, secondary 0.3%)
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) | 3.2% | [1.5%, 5.0%] | 2 |
| Regressions ❌ (secondary) | 2.1% | [2.1%, 2.1%] | 1 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -1.6% | [-1.6%, -1.6%] | 1 |
| All ❌✅ (primary) | 3.2% | [1.5%, 5.0%] | 2 |
Cycles
This benchmark run did not return any relevant results for this metric.
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 760.743s -> 762.964s (0.29%)
Artifact size: 337.11 MiB -> 339.32 MiB (0.66%)
bherrera pushed a commit to misttech/integration that referenced this pull request
Here are all the changes. I went through them one-by-one and confirmed that they should not be affecting us. In paritcular, we explicitly set rust.lld = false (because we want to use the lld that ships with clang), so the change in default does not affect us.
There have been changes to x.py since you last updated:
[INFO] New option build.lldb that will override the default lldb binary path used in debuginfo tests
- PR Link rust-lang/rust#124501
[INFO] The compiler profile now defaults to rust.debuginfo-level = "line-tables-only"
- PR Link rust-lang/rust#123337
[WARNING] rust.lld has a new default value of true on x86_64-unknown-linux-gnu. Starting at stage1, rust-lld will thus be this target's default linker. No config changes should be necessary.
- PR Link rust-lang/rust#124129
[WARNING] Removed dist.missing-tools configuration as it was deprecated long time ago.
- PR Link rust-lang/rust#125535
[WARNING] llvm.lld is enabled by default for the dist profile. If set to false, lld will not be included in the dist build.
- PR Link rust-lang/rust#126701
[WARNING] debug-logging option has been removed from the default tools profile.
- PR Link rust-lang/rust#127913
[INFO] the wasm-component-ld tool is now built as part of build.extended and can be a member of build.tools
- PR Link rust-lang/rust#127866
[INFO] Removed android-ndk r25b support in favor of android-ndk r26d.
- PR Link rust-lang/rust#120593
[WARNING] For tarball sources, default value for rust.channel will be taken from src/ci/channel file.
- PR Link rust-lang/rust#125181
[INFO] New option llvm.libzstd to control whether llvm is built with zstd support.
- PR Link rust-lang/rust#125642
[WARNING] ./x test --rustc-args was renamed to --compiletest-rustc-args as it only applies there. ./x miri --rustc-args was also removed.
- PR Link rust-lang/rust#128841
[INFO] The build.profiler option now tries to use source code from download-ci-llvm if possible, instead of checking out the src/llvm-project submodule.
- PR Link rust-lang/rust#129295
Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/recipes/+/1120078 Original-Revision: 27df37a30e50b14b9ffefc872b6997790f03d4ea GitOrigin-RevId: 341e222f002e36886b9960645b21faeaed633f90 Change-Id: Id1eb54a677a6f538bf7666d65b85d5fdba17ea42
Reviewers
Labels
Unstable option: debuginfo compression
Area: port run-make Makefiles to rmake.rs
Area: The testsuite used to check the correctness of rustc
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.
Relevant to the infrastructure team, which will review and decide on the PR/issue.