-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
kexec-tools: Only force ELFv2 on ppc64 when the platform settings aggree #422007
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+7
−5
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to make the patch optional? The next person updating the package might easily miss this to the disadvantage of powerpc users.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this patch file? Yes, because it breaks building for ELFv1 (which, eventually, I'm planning to make our default ABI on powerpc64, because it's less effort to get it to work properly). Doing this in a way that supports both ABI would require a custom patch, because upstream currently doesn't support different flags for different ABI targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually don't do conditional patches, because they end up breaking easily without being noticed during regular package bumps.
We're less strict for more "exotic" targets/platforms, see this example:
nixpkgs/pkgs/os-specific/linux/systemd/default.nix
Line 257 in 349024c
I think it's fine to pull it in conditionally, cause power64 is also somewhat niche, but I strongly recommend to start upstreaming this in a more generic fashion to avoid this breaking in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't elfv1 marked as legacy? (see rust-lang/rust#60617)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ELFv1 is considered legacy, but big-endian 64-bit POWER itself is kind of legacy too. Modern 64-bit POWER processors are usually run in little-endian mode, where ELFv2 is (IIRC) the only supported ABI. But 64-bit POWER started out as big-endian in Linux, ELFv1 was the current version for most of its lifetime, and glibc still defaults to ELFv1 unless explicitly told otherwise, so it's still the "default".
The ABI situation of powerpc64 in the libc's is as follows:
Certain (bad) assumptions have developed like ~ "on 64-bit POWER, the ABI is ELFv2 on LE and ELFv1 on BE". Patches like the one that gets shuffled around in this PR come mostly from musl-running distros - Void PPC when it was still around, Adelie Linux, Chimera Linux are all musl distros that I used to look towards for ELFv2 patches. All glibc-using distros supported powerpc64 that I could find (Debian, Gentoo) use the ELFv1 ABI that glibc defaults to. Some (Gentoo IIRC) attempted to switch their glibc to ELFv2 in the past, but it looks like they're still on ELFv1 as of today.
I gave up on ELFv2 when I got to Valgrind. I was unable to get it to accept & properly handle ELFv2 binaries produced by Nix, unless I built them against musl and used patchelf to replace our musl in the final binary with i.e. Adelie's musl. So either something is missing in the ELFv2 patch, or our toolchain just uniquely breaks Valgrind. The ELFv2 MR has been dormant for awhile upstream, so I'm not too optimistic about this support becoming official soon. In contrast, ELFv1 Valgrind has worked just fine.
There are more issues with ELFv2 on powerpc64. Most prebuilt binaries target ELFv1, so i.e. bootstrapping Rust, GHC etc gets more annoying, and would require cross-compilation between ABIs or something weird like that. I know Adelie has their own prebuilt, prepatched Rust toolchain artefacts on their GitLab instance that they use for Rust things. I imagine that those might not just magically work for us though. With GHC, I'm even less sure about the state of ELFv2 support. All of these issues naturally go away when we don't force ELFv2.
So in short, yes it's legacy, but it Just Works ™️, and is the path of least resistance on this platform.