Skip to content
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.

Sign up for GitHub

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

Jump to bottom

llvm: Fix compiler-rt missing sanitizers when using useLLVM #297144

Merged
merged 1 commit into from
Jul 29, 2024
Merged

llvm: Fix compiler-rt missing sanitizers when using useLLVM #297144

merged 1 commit into from
Jul 29, 2024

Conversation

s1341
Copy link
Contributor

@s1341 s1341 commented Mar 19, 2024

Attempts to address #97688 by refactoring the llvm/16 toolchain build to make more sense.

The build order is now:

{compiler-rt-no-libc}
{compiler-rt-no-libc, libc}
{compiler-rt-no-libc, libc, libc++}
{compiler-rt-no-libc, libc, libc++, compiler-rt-libc}
The names of the stages have been updated to reflect this.

This is a follow up for #267814 where I screwed up the rebase and managed to ping too many people. Apologies!

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.

@s1341 s1341 requested a review from RaitoBezarius as a code owner March 19, 2024 10:18
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 8.has: clean-up 8.has: package (new) This PR adds a new package labels Mar 19, 2024
@RossComputerGuy RossComputerGuy added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 15, 2024
@RossComputerGuy RossComputerGuy self-requested a review April 16, 2024 05:18
@Ericson2314 Ericson2314 force-pushed the fix_compiler_rt branch 4 times, most recently from eb2870d to a1f0bce Compare April 22, 2024 16:21
@Ericson2314 Ericson2314 marked this pull request as draft April 22, 2024 16:21
Ericson2314
Ericson2314 reviewed Apr 22, 2024
View reviewed changes
pkgs/development/compilers/llvm/common/compiler-rt/default.nix Outdated
Comment on lines 80 to 84
"-DCOMPILER_RT_BUILD_SANITIZERS=ON"
"-DCOMPILER_RT_BUILD_PROFILE=ON"
] ++ lib.optionals ((useLLVM && !haveLibc) || bareMetal || isMusl || isDarwinStatic) [
"-DCOMPILER_RT_BUILD_SANITIZERS=OFF"
"-DCOMPILER_RT_BUILD_PROFILE=OFF"
Copy link
Member

@Ericson2314 Ericson2314 Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For stuff like this, we're probably best of transitioning to

(lib.cmakeBool "COMPILER_RT_BUILD_SANITIZERS" condition-for-sanitizers)
(lib.cmakeBool "COMPILER_RT_BUILD_PROFILE" condition-for-profile)

i.e. instead of conditionally passing flags, we should always pass flags, and conditionally set the values the flags specify.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 22, 2024
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
@pwaller
Copy link
Contributor

pwaller commented Jun 16, 2024

I like the fix and general approach. I note that nixpkgs LLVM has changed a fair amount, so this will need reimplementing. Thankfully, you should only need to implement it once for all LLVM versions, since they were made to share 'common' nixpkgs sources.

@Ericson2314
Copy link
Member

I plan on re-implementing this now.

@github-actions github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS and removed 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related labels Jun 30, 2024
@Ericson2314 Ericson2314 mentioned this pull request Jul 12, 2024
Closed
13 tasks
@ofborg ofborg bot requested review from RossComputerGuy and Ericson2314 July 12, 2024 14:32
@Ericson2314 Ericson2314 marked this pull request as ready for review July 12, 2024 14:41
RossComputerGuy
RossComputerGuy reviewed Jul 12, 2024
View reviewed changes
Copy link
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of nixpkgs-review run on aarch64-linux 1

21 packages marked as broken and skipped:
  • bintoolsDualAs
  • darwin.autoSignDarwinBinariesHook
  • darwin.binutils
  • darwin.binutils-unwrapped
  • darwin.binutils-unwrapped.man
  • darwin.binutils.man
  • darwin.binutilsDualAs
  • darwin.binutilsDualAs-unwrapped
  • darwin.binutilsNoLibc
  • darwin.binutilsNoLibc.man
  • darwin.cctools
  • darwin.cctools-llvm
  • darwin.cctools-llvm.dev
  • darwin.cctools-llvm.man
  • darwin.cctools.dev
  • darwin.cctools.gas
  • darwin.cctools.man
  • darwin.postLinkSignHook
  • darwin.signingUtils
  • macdylibbundler
  • ns-3
72 packages failed to build:
  • aapt
  • adoptopenjdk-icedtea-web
  • apktool
  • betterbird
  • betterbird-unwrapped
  • betterbird-unwrapped.debug
  • diffoscope
  • diffoscope.dist
  • diffoscope.man
  • eyewitness
  • feishu
  • firefox
  • firefox-beta
  • firefox-beta-unwrapped
  • firefox-beta-unwrapped.debug
  • firefox-beta-unwrapped.symbols
  • firefox-devedition
  • firefox-devedition-unwrapped
  • firefox-devedition-unwrapped.debug
  • firefox-devedition-unwrapped.symbols
  • firefox-esr
  • firefox-esr-115-unwrapped
  • firefox-esr-115-unwrapped.debug
  • firefox-esr-115-unwrapped.symbols
  • firefox-esr-128
  • firefox-esr-128-unwrapped
  • firefox-esr-128-unwrapped.debug
  • firefox-esr-128-unwrapped.symbols
  • firefox-mobile
  • firefox-unwrapped
  • firefox-unwrapped.debug
  • firefox-unwrapped.symbols
  • firefoxpwa
  • floorp
  • floorp-unwrapped
  • floorp-unwrapped.debug
  • gnss-sdr
  • gnuradio
  • gnuradioPackages.osmosdr
  • gnuradioPackages.osmosdr.dev
  • irods
  • irods-icommands
  • libcxx
  • libcxx.dev
  • libcxxStdenv
  • librewolf
  • librewolf-unwrapped
  • librewolf-unwrapped.debug
  • llvmPackages_12.clangNoLibc
  • llvmPackages_12.clangNoLibcxx
  • llvmPackages_12.clangUseLLVM
  • llvmPackages_12.clangWithLibcAndBasicRtAndLibcxx
  • llvmPackages_12.compiler-rt-no-libc
  • llvmPackages_12.compiler-rt-no-libc.dev
  • llvmPackages_12.libcxx
  • llvmPackages_12.libcxx.dev
  • llvmPackages_12.libcxxClang
  • llvmPackages_12.libcxxStdenv
  • llvmPackages_12.libunwind
  • llvmPackages_12.libunwind.dev
  • pocl
  • python311Packages.pygccxml
  • python311Packages.pygccxml.dist
  • python312Packages.pygccxml
  • python312Packages.pygccxml.dist
  • sbt-with-scala-native
  • sitespeed-io
  • slimerjs
  • thunderbird
  • thunderbird-unwrapped
  • thunderbird-unwrapped.debug
  • thunderbird-unwrapped.symbols

@RossComputerGuy
Copy link
Member

llvmPackages_12.clangWithLibcAndBasicRtAndLibcxx fails to compile due to compiler-rt:

compiler-rt>     clang-12: warning: argument unused during compilation: '-nostartfiles' [-Wunused-command-line-argument]
compiler-rt>     Linking C executable cmTC_b0eb1
compiler-rt>     /nix/store/hyr25rm1gxiz04bbdvdk7a6291anq685-cmake-3.29.6/bin/cmake -E cmake_link_script CMakeFiles/cmTC_b0eb1.dir/link.txt --verbose=1
compiler-rt>     /nix/store/278ccfykw0mg3a4rbsql4s4jxwyjzcd8-clang-wrapper-12.0.1/bin/clang --target=aarch64-unknown-linux-gnu CMakeFiles/cmTC_b0eb1.dir/testCCompiler.c.o -o cmTC_b0eb1
compiler-rt>     /nix/store/1jbqn7z7rh2q23nda4r66hr601z0w753-binutils-2.42/bin/ld: cannot find -lc: No such file or directory
compiler-rt>     clang-12: error: linker command failed with exit code 1 (use -v to see invocation)
compiler-rt>     make[1]: *** [CMakeFiles/cmTC_b0eb1.dir/build.make:100: cmTC_b0eb1] Error 1
compiler-rt>     make[1]: Leaving directory '/build/compiler-rt-12.0.1.src/build/CMakeFiles/CMakeScratch/TryCompile-agZWJE'
compiler-rt>     make: *** [Makefile:127: cmTC_b0eb1/fast] Error 2

@Ericson2314
Copy link
Member

Thanks @RossComputerGuy. That made me look harder at the compiler-rt file again, and I found some things to fix. Looking forward to mass rebuild cleanups sometime later! :)

@RossComputerGuy
Copy link
Member

Looking forward to mass rebuild cleanups sometime later! :)

Sweet, that's working great so far. 13 building, 33 built, 69 to go.

RossComputerGuy
RossComputerGuy reviewed Jul 13, 2024
View reviewed changes
Copy link
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of nixpkgs-review run on aarch64-linux 1

21 packages marked as broken and skipped:
  • bintoolsDualAs
  • darwin.autoSignDarwinBinariesHook
  • darwin.binutils
  • darwin.binutils-unwrapped
  • darwin.binutils-unwrapped.man
  • darwin.binutils.man
  • darwin.binutilsDualAs
  • darwin.binutilsDualAs-unwrapped
  • darwin.binutilsNoLibc
  • darwin.binutilsNoLibc.man
  • darwin.cctools
  • darwin.cctools-llvm
  • darwin.cctools-llvm.dev
  • darwin.cctools-llvm.man
  • darwin.cctools.dev
  • darwin.cctools.gas
  • darwin.cctools.man
  • darwin.postLinkSignHook
  • darwin.signingUtils
  • macdylibbundler
  • ns-3
42 packages failed to build:
  • adoptopenjdk-icedtea-web
  • betterbird
  • betterbird-unwrapped
  • betterbird-unwrapped.debug
  • eyewitness
  • firefox
  • firefox-beta
  • firefox-beta-unwrapped
  • firefox-beta-unwrapped.debug
  • firefox-beta-unwrapped.symbols
  • firefox-devedition
  • firefox-devedition-unwrapped
  • firefox-devedition-unwrapped.debug
  • firefox-devedition-unwrapped.symbols
  • firefox-esr
  • firefox-esr-115-unwrapped
  • firefox-esr-115-unwrapped.debug
  • firefox-esr-115-unwrapped.symbols
  • firefox-esr-128
  • firefox-esr-128-unwrapped
  • firefox-esr-128-unwrapped.debug
  • firefox-esr-128-unwrapped.symbols
  • firefox-mobile
  • firefox-unwrapped
  • firefox-unwrapped.debug
  • firefox-unwrapped.symbols
  • firefoxpwa
  • floorp
  • floorp-unwrapped
  • floorp-unwrapped.debug
  • irods
  • irods-icommands
  • librewolf
  • librewolf-unwrapped
  • librewolf-unwrapped.debug
  • pocl
  • sitespeed-io
  • slimerjs
  • thunderbird
  • thunderbird-unwrapped
  • thunderbird-unwrapped.debug
  • thunderbird-unwrapped.symbols
30 packages built:
  • aapt
  • apktool
  • diffoscope
  • diffoscope.dist
  • diffoscope.man
  • feishu
  • gnss-sdr
  • gnuradio
  • gnuradioPackages.osmosdr
  • gnuradioPackages.osmosdr.dev
  • libcxx
  • libcxx.dev
  • libcxxStdenv
  • llvmPackages_12.clangNoLibc
  • llvmPackages_12.clangNoLibcxx
  • llvmPackages_12.clangUseLLVM
  • llvmPackages_12.clangWithLibcAndBasicRtAndLibcxx
  • llvmPackages_12.compiler-rt-no-libc
  • llvmPackages_12.compiler-rt-no-libc.dev
  • llvmPackages_12.libcxx
  • llvmPackages_12.libcxx.dev
  • llvmPackages_12.libcxxClang
  • llvmPackages_12.libcxxStdenv
  • llvmPackages_12.libunwind
  • llvmPackages_12.libunwind.dev
  • python311Packages.pygccxml
  • python311Packages.pygccxml.dist
  • python312Packages.pygccxml
  • python312Packages.pygccxml.dist
  • sbt-with-scala-native

@ofborg ofborg bot requested a review from RossComputerGuy July 13, 2024 01:14
@Ericson2314 Ericson2314 force-pushed the fix_compiler_rt branch 3 times, most recently from ac2c768 to 7bea7b5 Compare July 24, 2024 18:27
@Ericson2314
Copy link
Member

@RossComputerGuy The remaining changes are due to libcxx on Linux changing in an intentional way, and I tried some things and they still built fine. (Last push is just a rename to make something clearer, no hashes should be changed.)

If you want to run another nixpkgs-review, I think this is ready!

@RossComputerGuy
Copy link
Member

If you want to run another nixpkgs-review, I think this is ready!

Can't now but will do after work.

@Ericson2314
Copy link
Member

(After #329706, this should be an easy backport, and I believe it is safe too --- worse that happens is you get a more featureful compiler-rt than before, yay.)

@Ericson2314
Copy link
Member 

20 packages marked as broken and skipped:
bintoolsDualAs darwin.autoSignDarwinBinariesHook darwin.binutils darwin.binutils-unwrapped darwin.binutils-unwrapped.man darwin.binutils.man darwin.binutilsDualAs darwin.binutilsDualAs-unwrapped darwin.binutilsNoLibc darwin.binutilsNoLibc.man darwin.cctools darwin.cctools-llvm darwin.cctools-llvm.dev darwin.cctools-llvm.man darwin.cctools.dev darwin.cctools.gas darwin.cctools.man darwin.postLinkSignHook darwin.signingUtils macdylibbundler

8 packages failed to build:
citrix_workspace citrix_workspace_23_09_0 citrix_workspace_23_11_0 citrix_workspace_24_02_0 irods irods-icommands snapdragon-profiler upwork

44 packages built:
aapt apktool blackmagic-desktop-video diffoscope diffoscope.dist diffoscope.man discord discord-canary discord-development discord-ptb edgetpu-compiler feishu gnss-sdr gnuradio gnuradioPackages.osmosdr gnuradioPackages.osmosdr.dev gyroflow libcxx libcxx.dev libcxxStdenv llvmPackages_12.clangNoLibc llvmPackages_12.clangNoLibcxx llvmPackages_12.clangUseLLVM llvmPackages_12.clangWithLibcAndBasicRtAndLibcxx llvmPackages_12.compiler-rt-no-libc llvmPackages_12.compiler-rt-no-libc.dev llvmPackages_12.libcxx llvmPackages_12.libcxx.dev llvmPackages_12.libcxxClang llvmPackages_12.libcxxStdenv llvmPackages_12.libunwind llvmPackages_12.libunwind.dev maloader mdk-sdk ns-3 pocl premid python311Packages.pygccxml python311Packages.pygccxml.dist python312Packages.pygccxml python312Packages.pygccxml.dist sbt-with-scala-native teamspeak_client wrangler

I am not sure I did nixpkgs-review right, but this was my result. I think ti's a good result --- the things that did not build are unfree things with source code one needs to dowload manually. I think (once ofborg completes again) this is ready to be merged!

@Ericson2314 Ericson2314 merged commit 7043e14 into NixOS:master Jul 29, 2024
25 of 27 checks passed
@github-actions GitHub Actions
Copy link
Contributor

Successfully created backport PR for release-24.05:

@github-actions github-actions bot mentioned this pull request Jul 29, 2024
Merged
1 task
@RossComputerGuy
Copy link
Member

This partially broke the PR #329995 since valgrind does a static binary and sanitizers rely on getauxval which isn't in libc_nonshared.a.

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

@RaitoBezarius RaitoBezarius Awaiting requested review from RaitoBezarius

@alyssais alyssais Awaiting requested review from alyssais

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

@lovek323 lovek323 Awaiting requested review from lovek323

@rrbutani rrbutani Awaiting requested review from rrbutani

@dtzWill dtzWill Awaiting requested review from dtzWill

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@RossComputerGuy RossComputerGuy Awaiting requested review from RossComputerGuy

Assignees

@Ericson2314 Ericson2314

Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@s1341 @pwaller @Ericson2314 @RossComputerGuy @rrbutani @wegank