Enable zstd for debug compression. #125642

khuey · 2024-05-28T03:12:28Z

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

rustbot · 2024-05-28T03:12:36Z

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

khuey · 2024-05-28T04:35:35Z

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.

rustbot · 2024-05-28T04:35:39Z

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

lqd · 2024-05-28T09:07:35Z

How are you producing this zstd-compressed debuginfo without building your own rustc/llvm to add zstd support in -Zdebuginfo-compression?

khuey · 2024-05-28T13:55:05Z

-C link-arg=-Wl,--compress-debug-sections=zstd

lqd · 2024-05-28T14:16:21Z

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

khuey · 2024-05-28T14:38:52Z

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.

lqd · 2024-05-28T14:42:15Z

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.

workingjubilee · 2024-05-28T18:13:18Z

@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 · 2024-05-28T18:15:38Z

@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.

workingjubilee · 2024-05-28T18:31:34Z

Okay, "we want everything to just be rolled up into our LLVM" sounds fine to me.

onur-ozkan · 2024-05-28T19:44:53Z

src/bootstrap/src/core/build_steps/llvm.rs

-        // 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");


How about making this configurable in config.toml (off by default) instead of constantly setting it as "ON" ?

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.

I do not understand why we would make something configurable without a specific reason to do so.

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

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.

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.

onur-ozkan · 2024-05-29T05:24:59Z

If we enable zstd we should probably statically link it.

Once this is done, I am okay with landing it.

khuey · 2024-05-29T21:03:11Z

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.

khuey · 2024-05-30T01:21:55Z

Alright with llvm/llvm-project#93754 and those two additional changesets everything gets linked statically.

onur-ozkan · 2024-05-30T05:00:50Z

r? compiler

fee1-dead · 2024-05-30T09:56:57Z

r? compiler

Kobzol · 2024-08-09T10:06:26Z

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.

src/ci/docker/host-x86_64/x86_64-gnu-tools/Dockerfile

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.

khuey · 2024-08-09T13:22:50Z

I reorganized these into something halfway sensible.

Time for yet another try run!

Kobzol · 2024-08-09T13:56:42Z

Thanks. Let's try again.

@bors r+

bors · 2024-08-09T13:56:44Z

📌 Commit 8db318c has been approved by Kobzol

It is now in the queue for this repository.

bors · 2024-08-09T15:05:15Z

⌛ Testing commit 8db318c with merge 4029c72...

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

rust-log-analyzer · 2024-08-09T17:26:03Z

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

bors · 2024-08-09T17:26:32Z

💔 Test failed - checks-actions

Kobzol · 2024-08-09T17:29:31Z

That failure is not related to this PR.

@bors retry

bors · 2024-08-09T22:44:40Z

⌛ Testing commit 8db318c with merge 68d2e8a...

bors · 2024-08-10T01:11:43Z

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing 68d2e8a to master...

khuey · 2024-08-10T01:12:56Z

Huzzah!

Thanks everyone.

rust-timer · 2024-08-10T02:29:51Z

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

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

rustbot assigned onur-ozkan May 28, 2024

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) labels May 28, 2024

workingjubilee mentioned this pull request May 28, 2024

Support zstd-compressed ELF sections. rust-lang/backtrace-rs#626

Merged

khuey marked this pull request as ready for review May 28, 2024 04:35

onur-ozkan reviewed May 28, 2024

View reviewed changes

onur-ozkan 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 May 28, 2024

khuey force-pushed the zstd branch from 169f982 to c638c09 Compare May 29, 2024 16:55

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 30, 2024

rustbot assigned fee1-dead and unassigned onur-ozkan May 30, 2024

onur-ozkan 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 May 30, 2024

lqd reviewed Aug 9, 2024

View reviewed changes

src/ci/docker/host-x86_64/x86_64-gnu-tools/Dockerfile Outdated Show resolved Hide resolved

khuey added 4 commits August 9, 2024 05:55

Test --compress-debug-sections with rust-lld.

9f3376e

Add zlib and zstd dev packages for building lld.

7bb345e

Install zstd in dist builder image.

8db318c

Build libzstd from source because the EPEL package is built without fPIC.

khuey force-pushed the zstd branch from 4759087 to 8db318c Compare August 9, 2024 13:21

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 9, 2024

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 9, 2024

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 9, 2024

jieyouxu mentioned this pull request Aug 10, 2024

tests/run-make/dump-ice-to-disk is flakey on i686-mingw #128911

Open

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 10, 2024

bors merged commit 68d2e8a into rust-lang:master Aug 10, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable zstd for debug compression. #125642

Enable zstd for debug compression. #125642

khuey commented May 28, 2024 •

edited by Kobzol

Loading

rustbot commented May 28, 2024

khuey commented May 28, 2024

rustbot commented May 28, 2024

lqd commented May 28, 2024

khuey commented May 28, 2024

lqd commented May 28, 2024

khuey commented May 28, 2024

lqd commented May 28, 2024

workingjubilee commented May 28, 2024

nikic commented May 28, 2024

workingjubilee commented May 28, 2024

onur-ozkan May 28, 2024 •

edited

Loading

khuey May 28, 2024

workingjubilee May 28, 2024

onur-ozkan May 28, 2024

workingjubilee May 29, 2024 •

edited

Loading

khuey May 29, 2024

onur-ozkan commented May 29, 2024

khuey commented May 29, 2024

khuey commented May 30, 2024

onur-ozkan commented May 30, 2024

fee1-dead commented May 30, 2024

Kobzol commented Aug 9, 2024

khuey commented Aug 9, 2024

Kobzol commented Aug 9, 2024

bors commented Aug 9, 2024

bors commented Aug 9, 2024

rust-log-analyzer commented Aug 9, 2024

bors commented Aug 9, 2024

Kobzol commented Aug 9, 2024

bors commented Aug 9, 2024

bors commented Aug 10, 2024

khuey commented Aug 10, 2024

rust-timer commented Aug 10, 2024

	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

Enable zstd for debug compression. #125642

Enable zstd for debug compression. #125642

Conversation

khuey commented May 28, 2024 • edited by Kobzol Loading

rustbot commented May 28, 2024

khuey commented May 28, 2024

rustbot commented May 28, 2024

lqd commented May 28, 2024

khuey commented May 28, 2024

lqd commented May 28, 2024

khuey commented May 28, 2024

lqd commented May 28, 2024

workingjubilee commented May 28, 2024

nikic commented May 28, 2024

workingjubilee commented May 28, 2024

onur-ozkan May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan commented May 29, 2024

khuey commented May 29, 2024

khuey commented May 30, 2024

onur-ozkan commented May 30, 2024

fee1-dead commented May 30, 2024

Kobzol commented Aug 9, 2024

khuey commented Aug 9, 2024

Kobzol commented Aug 9, 2024

bors commented Aug 9, 2024

bors commented Aug 9, 2024

rust-log-analyzer commented Aug 9, 2024

bors commented Aug 9, 2024

Kobzol commented Aug 9, 2024

bors commented Aug 9, 2024

bors commented Aug 10, 2024

khuey commented Aug 10, 2024

rust-timer commented Aug 10, 2024

Overall result: no relevant changes - no action needed

khuey commented May 28, 2024 •

edited by Kobzol

Loading

onur-ozkan May 28, 2024 •

edited

Loading

workingjubilee May 29, 2024 •

edited

Loading