Enable zstd for debug compression. by khuey · Pull Request #125642 · rust-lang/rust

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

khuey · 2024-08-08T18:11:49Z

Let's try that.

Kobzol · 2024-08-08T20:21:03Z

Interesting. We used LLVM_ENABLE_ZLIB before on that runner for LLVM, but even though zlib was not found, it did not matter. That seems to show that zlib wasn't actually used during the LLVM compilation, as it is only probably used for tools. And the default value is ON (use zlib if found), so setting the flag was actually a no-op, wonderful.

What I don't understand though is why this breaks the compilation of LLD, this almost seems like a LLVM CMake bug. Its documentation clearly states that ON means "enable if zlib is found" (https://llvm.org/docs/CMake.html), but here we specify ON, zlib is clearly not found, but compilation still fails.

Kobzol · 2024-08-08T20:30:33Z

Actually it's even weirder, because ON should be the default value, so setting it explicitly shouldn't do anything.

khuey · 2024-08-08T21:04:47Z

Yeah, that does seem like a bug in LLVM's CMakeFiles. Can't say I really care to get to the bottom of it.

Kobzol · 2024-08-08T21:07:23Z

I suppose that we could just remove the ZLIB configuration altogether, and just rely on the fact that the default will be "use ZLIB if it is available". We should just check that without setting that flag (and having zlib available when building), LLD will support zlib compression.

nikic · 2024-08-08T21:24:54Z

The special handling for LLVM_ENABLE_ZLIB=ON only exists inside LLVM. It will then export the correct value from the LLVM cmake module depending on whether it was found, and that will then be inherited by other subprojects like lld. However, if you do a standalone build of lld and explicitly set the variable, then it will be overridden.

As this only affects the largely unsupported standalone build configuration, you should definitely not hope on an LLVM-side fix for this :)

khuey · 2024-08-08T23:27:52Z

I have no idea how it works but the rust-lld shipped with nightly supports zlib compression so I've reverted that part.

Kobzol · 2024-08-09T07:24:02Z

@bors try

Kobzol · 2024-08-09T07:25:07Z

As this only affects the largely unsupported standalone build configuration, you should definitely not hope on an LLVM-side fix for this :)

Thanks for providing the context! I didn't know that building the tools in a "standalone" mode is mostly unsupported. Do you suggest that we should switch from building LLD directly to building LLVM with LLVM_ENABLE_PROJECTS=lld?

bors · 2024-08-09T07:25:13Z

⌛ Trying commit 4759087 with merge f568b13...

bors · 2024-08-09T08:44:43Z

☀️ Try build successful - checks-actions
Build commit: f568b13 (f568b137b285620fa919275d76c5c02e76baa796)

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

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

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.

jieyouxu mentioned this pull request Aug 10, 2024

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

Closed

wesleywiser mentioned this pull request Jan 2, 2026

Stabilize -Zdebuginfo-compression as -Cdebuginfo-compression #150625

Open

	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

Uh oh!

Conversation

khuey commented May 28, 2024 • edited by Kobzol Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 28, 2024

Uh oh!

khuey commented May 28, 2024

Uh oh!

rustbot commented May 28, 2024

Uh oh!

lqd commented May 28, 2024

Uh oh!

khuey commented May 28, 2024

Uh oh!

lqd commented May 28, 2024

Uh oh!

khuey commented May 28, 2024

Uh oh!

lqd commented May 28, 2024

Uh oh!

workingjubilee commented May 28, 2024

Uh oh!

nikic commented May 28, 2024

Uh oh!

workingjubilee commented May 28, 2024

Uh oh!

onur-ozkan May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan commented May 29, 2024

Uh oh!

khuey commented May 29, 2024

Uh oh!

khuey commented May 30, 2024

Uh oh!

onur-ozkan commented May 30, 2024

Uh oh!

fee1-dead commented May 30, 2024

Uh oh!

khuey commented Aug 8, 2024

Uh oh!

Kobzol commented Aug 8, 2024

Uh oh!

Kobzol commented Aug 8, 2024

Uh oh!

khuey commented Aug 8, 2024

Uh oh!

Kobzol commented Aug 8, 2024

Uh oh!

nikic commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khuey commented Aug 8, 2024

Uh oh!

Kobzol commented Aug 9, 2024

Uh oh!

Kobzol commented Aug 9, 2024

Uh oh!

bors commented Aug 9, 2024

Uh oh!

bors commented Aug 9, 2024

Uh oh!

Kobzol commented Aug 9, 2024

Uh oh!

Uh oh!

khuey commented Aug 9, 2024

khuey commented May 28, 2024 •

edited by Kobzol

Loading

onur-ozkan May 28, 2024 •

edited

Loading

workingjubilee May 29, 2024 •

edited

Loading

nikic commented Aug 8, 2024 •

edited

Loading