Skip to content

Comments

clang: don't set -march for overridden target#291901

Merged
vcunat merged 1 commit intoNixOS:staging-nextfrom
alyssais:clang-march
Mar 1, 2024
Merged

clang: don't set -march for overridden target#291901
vcunat merged 1 commit intoNixOS:staging-nextfrom
alyssais:clang-march

Conversation

@alyssais
Copy link
Member

Description of changes

If -target is explicitly passed to clang, we shouldn't pass our -march value for the default target, because it probably won't exist for the target being used. Up until now, clang has been lenient with this, but it's a hard error with clang 17, so since gcc.arch is always set on aarch64, fixing this is a hard requirement for upgrading our default clang to 17.

Before (with clang 17 on aarch64-linux):

$ clang -target bpf -c -o /dev/null test.bpf.c
clang: warning: ignoring '-fstack-protector-strong' option as it is not currently supported for target 'bpf' [-Woption-ignored]
clang: error: unsupported option '-march=' for target 'bpf'
clang: warning: argument unused during compilation: '--gcc-toolchain=/nix/store/cngak08nb1yk44gnjipv5rg1ahh1xkax-gcc-13.2.0' [-Wunused-command-line-argument]

After:

$ clang -target bpf -c -o /dev/null test.bpf.c
clang: warning: ignoring '-fstack-protector-strong' option as it is not currently supported for target 'bpf' [-Woption-ignored]
clang: warning: argument unused during compilation: '--gcc-toolchain=/nix/store/cngak08nb1yk44gnjipv5rg1ahh1xkax-gcc-13.2.0' [-Wunused-command-line-argument]

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@alyssais alyssais requested a review from vcunat February 27, 2024 18:35
@alyssais alyssais requested a review from bobrik February 27, 2024 18:40
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Feb 27, 2024
@jys1670
Copy link
Contributor

jys1670 commented Feb 27, 2024

This issue seems similar (user-specified -march in gcc is troublesome)
#290621
Perhaps this change should not be limited to clang only (if possible)

@alyssais
Copy link
Member Author

Perhaps this change should not be limited to clang only (if possible)

This change wouldn't make any sense for GCC, because GCC is a single-target compiler. There's no overriding of targets like there is in GCC. That issue is something else.

@delroth delroth added 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Feb 27, 2024
@alyssais
Copy link
Member Author

@ofborg build ananicy-cpp below dae pwru suricata

@alyssais
Copy link
Member Author

Can somebody please smoke test this on Darwin, since OfBorg can't?

@bobrik
Copy link
Contributor

bobrik commented Feb 29, 2024

I checked out this PR and was able to build jq on aarch64-darwin:

ivan@laptop: ~/projects/nixpkgs on clang-march
$ nix-env --install --attr nixpkgs.jq
installing 'jq-1.7.1'
these 618 derivations will be built:
...

It's currently most of the way through:

$ nix-env --install --attr nixpkgs.jq --dry-run 2>&1 | head -n20
(dry run; not doing anything)
installing 'jq-1.7.1'
these 227 derivations will be built:
  /nix/store/v0k0a2q75cp09vzgxlf9ljdy62i4xbgm-llvm-16.0.6.drv
  /nix/store/jsmvqy75x060r9wwvik66gdzkiwfxgjq-clang-at-least-16-LLVMgold-path.patch.drv
  /nix/store/sk7czh1y2811h6acpa1xkka5g1p4ljx2-clang-16.0.6.drv
  /nix/store/9r2j8lngs0qhimj5hpaybml8qhhwd76z-bootstrap-stage4-clang-wrapper-16.0.6.drv
  /nix/store/1bfi7f39j73i2xfnimygfj6syagxb021-bootstrap-stage4-stdenv-darwin.drv
  /nix/store/0482ggy373hc1z9rhsmf1l98idrh1jga-pcre2-10.42.drv
  /nix/store/sdjk4kh0d7jfhmvwgjppmzl5f9bvf9j3-python3-3.11.7.drv
  /nix/store/g0sbk2vwfan7idkiszzhgqd9zmn7nr48-python3.11-bootstrap-flit-core-3.9.0.drv
  /nix/store/hib9phkaann70xi1i0whbcj5c7hcrpfj-python3.11-bootstrap-installer-0.7.0.drv
  /nix/store/62kcxs66n1fwrrnx5q66z2ynq84n0yxj-python3.11-bootstrap-packaging-23.2.drv
  /nix/store/299mwim0n0dydjfvjd4da9xqacbc1hsq-python-runtime-deps-check-hook.sh.drv
  /nix/store/1cakc97i4ldrb5zlbvh36y6jrs7vwd73-python3.11-bootstrap-packaging-23.2.drv
  /nix/store/d794wp8q6c7pz9nac75h68a0p87nz8m0-python3.11-bootstrap-tomli-2.0.1.drv
  /nix/store/gmm4167l7d08if6xv8n15myphpfix6li-python3.11-bootstrap-pyproject-hooks-1.0.0.drv
  /nix/store/7s27hxsxbzjlywqzab16lj7psdbzl7vj-python3.11-bootstrap-build-1.0.3.drv
  /nix/store/glnv4j89kw8kp2fib1x0fbc68mqya32s-pypa-build-hook.sh.drv
  /nix/store/hhgcvawjp0xq2xpkjpm68l05g7bbn12g-pypa-install-hook.drv

Do you have a more specific test in mind?

@alyssais
Copy link
Member Author

alyssais commented Feb 29, 2024

Not really — I just needed to know that I hadn't somehow completely broken the compiler wrapper. That sounds like a decent test — let me know when it's done?

@bobrik
Copy link
Contributor

bobrik commented Feb 29, 2024

It ran to completion with no troubles.

If -target is explicitly passed to clang, we shouldn't pass our -march
value for the default target, because it probably won't exist for the
target being used.  Up until now, clang has been lenient with this,
but it's a hard error with clang 17, so since gcc.arch is always set
on aarch64, fixing this is a hard requirement for upgrading our
default clang to 17.

Before (with clang 17 on aarch64-linux):

	$ clang -target bpf -c -o /dev/null test.bpf.c
	clang: warning: ignoring '-fstack-protector-strong' option as it is not currently supported for target 'bpf' [-Woption-ignored]
	clang: error: unsupported option '-march=' for target 'bpf'
	clang: warning: argument unused during compilation: '--gcc-toolchain=/nix/store/cngak08nb1yk44gnjipv5rg1ahh1xkax-gcc-13.2.0' [-Wunused-command-line-argument]

After:

	$ clang -target bpf -c -o /dev/null test.bpf.c
	clang: warning: ignoring '-fstack-protector-strong' option as it is not currently supported for target 'bpf' [-Woption-ignored]
	clang: warning: argument unused during compilation: '--gcc-toolchain=/nix/store/cngak08nb1yk44gnjipv5rg1ahh1xkax-gcc-13.2.0' [-Wunused-command-line-argument]
@delroth delroth removed the 12.approvals: 2 This PR was reviewed and approved by two persons. label Mar 1, 2024
@alyssais alyssais changed the base branch from staging to staging-next March 1, 2024 10:29
@vcunat vcunat merged commit 0e4d8e9 into NixOS:staging-next Mar 1, 2024
@alyssais alyssais deleted the clang-march branch March 1, 2024 10:32
@ghost
Copy link

ghost commented Mar 4, 2024

looks like this might be effecting the swift aarch64-darwin build? https://hydra.nixos.org/build/251794147

Run Build Command(s): /nix/store/40wywa8y2nsgws1lwj7nhmkbbsx1arq4-ninja-1.11.1/bin/ninja -v cmTC_809f1
[1/1] : && /private/tmp/nix-build-swift-5.8.drv-0/build/swift/bin/swiftc -j 8 -num-threads 8 -emit-executable -o cmTC_809f1 -emit-dependencies  -output-file-map CMakeFiles/cmTC_809f1.dir//output-file-map.json  /tmp/nix-build-swift-5.8.drv-0/build/swift-concurrency-backdeploy/CMakeFiles/CMakeScratch/TryCompile-DsXkpX/main.swift    && :
FAILED: cmTC_809f1 CMakeFiles/cmTC_809f1.dir/main.swift.o 
: && /private/tmp/nix-build-swift-5.8.drv-0/build/swift/bin/swiftc -j 8 -num-threads 8 -emit-executable -o cmTC_809f1 -emit-dependencies  -output-file-map CMakeFiles/cmTC_809f1.dir//output-file-map.json  /tmp/nix-build-swift-5.8.drv-0/build/swift-concurrency-backdeploy/CMakeFiles/CMakeScratch/TryCompile-DsXkpX/main.swift    && :
<unknown>:0: error: unknown argument: '-march=armv8.3-a+crypto+sha2+aes+crc+fp16+lse+simd+ras+rdm+rcpc'
ninja: build stopped: subcommand failed.

@alyssais
Copy link
Member Author

alyssais commented Mar 4, 2024

I don't really have any capacity/ability to investigate Darwin, I'm afraid. I see that the Swift build involves building and wrapping its own Clang. Before, -march wasn't set on aarch64-darwin at all, because GCC and Clang's system names are incompatible. Now it is set, but only for Clang. We could revert to not setting it on aarch64-darwin at all, but it seems a shame to do that when it only seems to cause problems for Swift…

@ghost
Copy link

ghost commented Mar 5, 2024

cc swift maintainers: @dtzWill @trepetti @dduan @Trundle @stephank

@vcunat
Copy link
Member

vcunat commented Mar 8, 2024

Also aarch64-linux: https://hydra.nixos.org/build/251767188

@softinio
Copy link
Member

@vcunat This seems to have been a breaking change for Darwin/apple sliicon users. Is anyone actively working on fix?

I would be happy to help but will need some guidance.

would a revert for now be an option?

@vcunat
Copy link
Member

vcunat commented Mar 13, 2024

I'm not aware of anyone working on a fix, but this seems a good place to ask. Perhaps specify what in particular is broken.

@ghost
Copy link

ghost commented Mar 16, 2024

have a for swfit by adding an argument to disable the -march arg in cc-wrapper. #296082

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants