Build LLVM with support for compression #95545
Closed
Conversation
This file contains 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
Through what seems to be an oversight we don't install libz in the aarch64 Docker image, which causes LLVM, and in turn both rustc and llvm-tools, to be built without support for compressing PGO and coverage files (and probably other things too). This patch rectifies that oversight so that aarch64 joins x86_64 in having compression support. Since we do this for both what writes and what reads these things, it doesn't usually manifest as an issue (beyond larger-than-necessary files). But it does mean that a profraw file written by rustc on x86_64 cannot be read by Rust's llvm-profdata on aarch64. It shows up in other situations too, such as if someone built rustc from source with zlib available on aarch64, but is using llvm-tools from rustup. So it feels worthwhile to fix. And for Google-ability, this tends to manifest with errors like: ``` profile uses zlib compression but the profile reader was built without zlib support ``` Note that we install `zlib1g-dev`, not `lib32z1-dev` as in the x86_64 image, since we're only building for 64-bit on aarch anyway.
|
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Are the build logs available for the CI builds? It's kind of hard to check whether this worked for me locally, but it should be immediately visible in the logs as, for example, |
|
Here's the Ubuntu package listing, if helpful: https://packages.ubuntu.com/focal/zlib1g-dev |
|
You can hunt down a builder here - https://github.com/rust-lang-ci/rust/actions (auto branch). Can you check that we do this for other tier 1 platforms as well? I think it makes sense to try to patch all of them as best we can; ideally we'd at least know and maybe document cases where it's not done (similar to e.g. missing stack overflow protection and other such historical(?) cases). |
|
FWIW this was explicitly disabled for Tier 1 This was different case though since here we have control over zlib library. |
|
Ah, right, I'm recalling that now. I think it may make sense to explicitly note that somewhere (e.g., the platform support page). |
|
@Mark-Simulacrum I think that'll only work for builds that actually ran as part of CI. But the aarch64 build doesn't run as CI normally I don't think? |
|
Looking at the other Tier 1 targets, there's Windows (which we're ignoring), macOS (which doesn't seem to have a Docker build?), and i686 which has zlib, so I think we're good. One thing I did notice was that there's also host-x86_64/dist-aarch64-linux. How does that differ from host-aarch64/aarch64-gnu? Should I update that one too? It seems to install mostly through cross-apt-packages.sh, but updating that feels like it'd affect other targets too? |
Not sure what you specifically mean - all of our builds run either never or during every auto branch CI build. The dist builder is what ships artifacts, the one that runs on native hardware builds artifacts and tests them but doesn't upload anything anywhere. I guess that covers the Linux targets, so we're probably good. |
|
What I meant was that CI doesn't run the host-aarch64 build, at least from what I can tell, which is what I'd need the logs from.
Hmm, that's interesting. I'm not sure how we grab the aarch64 version of zlib on x86_64 for cross-compilation 🤔 We may have to build it from source to get that to work. |
|
Which in turn means we'll need the zlib source tarball to be hosted on ci-mirrors.rust-lang.org like the OpenSSL and curl ones are. But I suppose I can use the upstream URL for now. |
|
I added a step to build zlib from source and cross-compile it for aarch64. It looks like the |
|
Why are the differences in the shebang style: |
|
I just changed them to be consistent with what was in the |
Debian multiarch can do that. |
|
@joshtriplett Ah, nice! Updated to use that instead. |
|
@bors r+ rollup=iffy |
|
📌 Commit fa7807b has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=1 |
|
⌛ Testing commit fa7807b with merge 266f08a7595734dee4b56bcebad4027bdbce6c7a... |
bors
added
S-waiting-on-review
|
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
Re-assigning to @joshtriplett to see if you might have a recommendation for how to resolve the problem here (per #95545 (comment)) |
|
I did a bors resync and this seems to have gotten reapproved, sorry for the noise @bors r- |
bors
added
S-waiting-on-author
compiler-errors
added
S-waiting-on-review
|
@bors r- |
bors
added
S-waiting-on-author
|
@bors ping |
|
😪 I'm awake I'm awake |
|
@bors retry |
bors
added
S-waiting-on-bors
|
@bors r- |
bors
added
S-waiting-on-author
|
Oops, sorry for the noise. |
apiraino
added
the
T-infra
|
☔ The latest upstream changes (presumably #95026) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@jonhoo any updates on this? thanks |
|
@jonhoo @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
|
Sorry for dropping the ball on this. Between moving and switching jobs, this became down-prioritized, and eventually I just dropped it altogether. I still think it's worthwhile to land a change like this to get smaller coverage files on aarch64, but I'm no longer in a position to push that forward myself as I don't have a direct need for it any more. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Through what seems to be an oversight we don't install libz in the
aarch64 Docker image, which causes LLVM, and in turn both rustc and
llvm-tools, to be built without support for compressing PGO and coverage
files (and probably other things too). This patch rectifies that
oversight so that aarch64 joins x86_64 in having compression support.
Since we do this for both what writes and what reads these things, it
doesn't usually manifest as an issue (beyond larger-than-necessary
files). But it does mean that a profraw file written by rustc on x86_64
cannot be read by Rust's llvm-profdata on aarch64. It shows up in other
situations too, such as if someone built rustc from source with zlib
available on aarch64, but is using llvm-tools from rustup. So it feels
worthwhile to fix.
And for Google-ability, this tends to manifest with errors like:
Note that we install
zlib1g-dev, notlib32z1-devas in the x86_64image, since we're only building for 64-bit on aarch anyway.