Fix separate-debug-info with lld#218301
Merged
mweinelt merged 3 commits intoNixOS:stagingfrom Mar 4, 2023
Merged
Conversation
rrbutani
requested review from
Ericson2314,
matthewbauer and
mweinelt
as code owners
NixOS#146275 has more discussion on this; the abridged version is that `lld` defaults to using `--build-id=fast` while GNU `ld` defaults to `--build-id=sha1`. These differ in length and so `separate-debug-info.sh`, as of this writing, errors on `lld`'s shorter `--build-id=fast`-generated hashes. `lld` offers the following `build-id` styles: - UUID (random; fast but bad for reproducibility) - fast (xxhash; fast but shorter hashes) - user provided hexstring - SHA1 - MD5 GNU `ld` supports the latter three options, `mold` supports all of these plus SHA256. UUID is out because it's not reproducible, fast isn't supported by GNU `ld` Using a nix provided (sourced from the output base hash) hash as the `build-id` seems tempting but would require a little extra work (we have to include some characteristic of the binary being hashed so that binaries within a derivation still have unique hashes; it seems easy to get this wrong; i.e. a path based approach would make two otherwise identical binaries that reside at different paths have different `build-id` hashes) That leaves SHA1 and MD5 and GNU `ld` already defaults to the former. This commit adds `$NIX_BUILD_ID_STYLE` as an escape hatch, in case any packages have strong opinions about which hash to use. ---- Note that if/when NixOS#146275 goes through, this change can be reverted if linker speed is a priority.
This was disabled here: NixOS@b86e62d#diff-282a02cc3871874f16401347d8fadc90d59d7ab11f6a99eaa5173c3867e1a160 h/t to @teh: NixOS@b86e62d#commitcomment-77916294 for pointing out that the failure that @matthewbauer was seeing was caused by the `separate-debug-info.sh` `build-id` length requirement that NixOS#146275 will relax `lld` has had `--build-id` support dating back to LLVM4: https://reviews.llvm.org/D18091 This predates every `llvmPackages_` version currently in nixpkgs (and certainly every version actually still used in `useLLVM` stdenvs) so with the previous commit (asking `ld` for sufficiently long SHA1 hashes) I think we can safely enable `separateDebugInfo` when using LLVM bintools.
With the previous two commits, lld/LLVM stdenvs should now produce `build-id`s that satisify `separate-debug-info.sh`
rrbutani
force-pushed
the
fix/separate-debuginfo-with-lld
branch
from
fa18375 to
45e5873
Compare
ofborg
bot
added
10.rebuild-darwin-stdenv
ofborg
bot
requested review from
edolstra,
lovesegfault,
nbp,
squalus and
vcunat
ofborg
bot
added
10.rebuild-darwin: 501+
12 tasks
trofi
approved these changes
Feb 26, 2023
lovesegfault
approved these changes
Feb 26, 2023
symphorien
approved these changes
Feb 27, 2023
Member
symphorien
left a comment
symphorien
left a comment
There was a problem hiding this comment.
sl built with pkgsLLVM.stdenv and separate debug info and firefox both have working debuginfo in gdb.
| # to SHA1. | ||
| if [ "$NIX_SET_BUILD_ID_@suffixSalt@" = 1 ] && ! (( "$relocatable" )); then | ||
| extraAfter+=(--build-id) | ||
| extraAfter+=(--build-id="${NIX_BUILD_ID_STYLE:-sha1}") |
Member
There was a problem hiding this comment.
Is there some ld-wrapper doc we should update with NIX_BUILD_ID_STYLE?
Contributor
Author
There was a problem hiding this comment.
Oh good point!
Maybe the separateDebugInfo section in the manual is a good place for this? It actually already talks about how build-id is a SHA1 hash 🙂
Member
There was a problem hiding this comment.
Please write some documentation in a follow-up PR!
figsoda
added
12.approvals: 3+
rrbutani
added
the
6.topic: llvm/clang
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Description of changes
This PR ultimately solves the same problem as #146275 but takes a slightly different approach: here we get
lldto emit SHA1 hashes instead of relaxing the length requirementseparate-debug-info.shhas.This approach (and this PR) is complementary to #146275's but if #146275 is merged we may want to switch back to letting
lld/mold/etc use faster hashes if we wish to prioritize build time (link speed) over the (low) potential for collisions.The commit messages within and #146275 have more discussion about this.
This PR also goes and:
separateDebugInfofor derivations using auseLLVMbased stdenvseparate-debug-info.shbeing unhappy withlld's shorter hashesThese fixes are applicable with #146275 or the first commit of this PR (or both).
h/t to @teh and @symphorien
Closes #22840
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)