cc-wrapper: keep machine cflags if the same target is passed #391944
Merged
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
|
Sounds sensible. Could you split |
in preparation for next commit
12b0e8a/6f756b4 work around clang errors when a nix-wrapped compiler is used like a multi-target compiler. it does so by dropping the machineFlags whenever -target/--target is found in the arguments. however this breaks compiler-rt, which is designed to acommodate multi-target builds and always passes `--target`. in its packaging we make sure to only build it for the target we need, essentially disabling the multi-target aspect, but because it still passes --target, the machineFlags are dropped and compiler-rt could end up being for an invalid ABI, producing an unusable stdenv.cc. we could manually pass machineFlags to compiler-rt's cmake build, but IMO it makes more sense to tolerate --target arguments whose value coincides with the wrapper's, keeping the machineFlags in this case.
mildsunrise
force-pushed
the
fix-llvm-stdenv-rt-cflags
branch
from
070933d to
220608b
Compare
|
done |
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.
Since #291901 / #317273, compiler-rt doesn't honor machine cflags. This can yield unusable LLVM stdenvs, for example this one even fails to build:
nix-build \ --arg crossSystem '{ config = "riscv32-unknown-linux-gnu"; gcc.arch = "rv32ima"; useLLVM = true; }' \ -A stdenvThis is because compiler-rt always passes
--target, causing machine flags to be dropped.Reading comments from @alyssais, @emilazy and #379593, I think there is general agreement that it's okay for things like compiler-rt to expect a multi-target compiler and pass
--target. #379593 dropped the warning when a--targetmatching the wrapper's is passed. I believe the most natural behavior is for the machine flags to be kept in that case, too, so thatproduce the same binaries. WDYT?
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.