-
-
Notifications
You must be signed in to change notification settings - Fork 18k
ghc: silence error on non-x86_64 with clang #251447
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
Conversation
When the stdenv is using a newer clang, it fails on non-x86_64 platforms due to an unused `popcount16` function, which is treated as an error. This only affects GHC 9.4.x, which I assume is due to a quirk of its build system that GHC 8.8.x, 8.10.x, 9.2.x, and 9.6.x do not share.
| # Silence unused function warning due to `popcount16` being defined on all platforms but used only | ||
| # on x86_64. This results in an error due to `-Werror`. | ||
| env.NIX_CFLAGS_COMPILE = lib.optionalString (stdenv.cc.isClang && !targetPlatform.isx86_64) "-Wno-unused-function"; |
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.
This seems reasonable.
I have two questions:
- My understanding is that GHC has a single version (or a range of versions) of LLVM that it supports. Would a better fix here be hard-coding the LLVM version used in
pkgs/top-level/haskell-packages.nix? We already do this for a few of our compilers, if I recall correctly. - In theory it is alright to merge this before llvmPackages: 11 -> 16 on Darwin and Linux #241692 is merged in? Even if we end up hard-coding the LLVM version in
pkgs/top-level/haskell-packages.nixto something earlier than 16, then the changes in this PR won't negatively effect anything, right? This is basically just silencing a warning, right?
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.
-Wno-unused-function is supported by at least clang 3. Otherwise, it’d need to be added conditionally based on the clang version (e.g., #253740). Applying this patch without #241692 shouldn’t negatively affect anything.
My understanding is that GHC has a single version (or a range of versions) of LLVM that it supports. Would a better fix here be hard-coding the LLVM version used in pkgs/top-level/haskell-packages.nix? We already do this for a few of our compilers, if I recall correctly.
GHC 9.4.x does specify an LLVM version, but isn’t that for the LLVM backend? When I look at the result of the preConfigure script, it’s setting CC, etc to clang 16.
However, when I drop this patch from my clang 16 branch (to get the specific error and check the build process), it builds successfully. The log shows numerous “errors” related to strict prototypes and this unused function, but they’re being treated as warnings. 🤨
I have no idea what changed or why it started working.
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.
GHC 9.4.x does specify an LLVM version, but isn’t that for the LLVM backend?
Ah, I see, so you're saying that there is a difference between the clang version used for compiling GHC, and the version passed as the LLVM backend?
I'm not very familiar with area, so maybe I should let someone else do the review here.
when I drop this patch from my clang 16 branch (to get the specific error and check the build process), it builds successfully
Ah, I see. I wonder what changed for you?
So I guess this PR might end up not being needed?
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.
Ah, I see, so you're saying that there is a difference between the clang version used for compiling GHC, and the version passed as the LLVM backend?
That appears to be the case.
Ah, I see. I wonder what changed for you?
The only thing that’s changed system-wise is I’m now running the macOS 14 beta, but I don’t know why that would matter.
So I guess this PR might end up not being needed?
Possibly not. I can close it for now and reopen if it causes issues on Hydra. It’s very peculiar. In July, I had all the errors causing problems. When I circled back to test again in August, only GHC 9.4.x was failing (and even it’s building now). That was prior to my installing the macOS 14 beta. I was still on 13.5 at that time.
toonn
left a comment
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.
Seems like the staging commit this is based on has broken something related to Clang 16.
-- Check for working C compiler: /nix/store/a8n2mxmb2pv5z9zmxw53v6r7zx3[21/1884]
ng-wrapper-16.0.6/bin/clang - broken
CMake Error at /nix/store/96yvirb7vsa847xgrj8kp9x3fw4gixyi-cmake-3.26.4/share/cm
ake-3.26/Modules/CMakeTestCCompiler.cmake:67 (message): The C compiler
"/nix/store/a8n2mxmb2pv5z9zmxw53v6r7zx3nsh7y-clang-wrapper-16.0.6/bin/clang"
is not able to compile a simple test program.
It fails with the following output:
Change Dir: /tmp/nix-build-libcxxabi-16.0.6.drv-0/libcxxabi-src-16.0.6/runti
mes/build/CMakeFiles/CMakeScratch/TryCompile-LprYEu
Run Build Command(s):/nix/store/ypfdjxc16m3g9iqiw8sjk1mjxixgszg7-ninja-1.11.
1/bin/ninja -v cmTC_56d1d && [1/2] /nix/store/a8n2mxmb2pv5z9zmxw53v6r7zx3nsh7y-clang-wrapper-16.0.6/bin/clang -MD -MT CMakeFiles/cmTC_56d1d.dir/testCCompiler
.c.o -MF CMakeFiles/cmTC_56d1d.dir/testCCompiler.c.o.d -o CMakeFiles/cmTC_56d1d.dir/testCCompiler.c.o -c /tmp/nix-build-libcxxabi-16.0.6.drv-0/libcxxabi-src-16.
0.6/runtimes/build/CMakeFiles/CMakeScratch/TryCompile-LprYEu/testCCompiler.c [2/2] : && /nix/store/a8n2mxmb2pv5z9zmxw53v6r7zx3nsh7y-clang-wrapper-16.0.6/
bin/clang -Wl,-search_paths_first -Wl,-headerpad_max_install_names -nostdlib CM
akeFiles/cmTC_56d1d.dir/testCCompiler.c.o -o cmTC_56d1d && :
FAILED: cmTC_56d1d
: && /nix/store/a8n2mxmb2pv5z9zmxw53v6r7zx3nsh7y-clang-wrapper-16.0.6/bin/cl
ang -Wl,-search_paths_first -Wl,-headerpad_max_install_names -nostdlib CMakeFil
es/cmTC_56d1d.dir/testCCompiler.c.o -o cmTC_56d1d && :
ld: dynamic main executables must link with libSystem.dylib for architecture
arm64
clang-16: error: linker command failed with exit code 1 (use -v to see invoc
ation)
ninja: build stopped: subcommand failed.
CMake will not be able to correctly generate this project.
Call Stack (most recent call first):
CMakeLists.txt:10 (project)
-- Configuring incomplete, errors occurred!
error: builder for '/nix/store/aa6rm3h1jibcsgn2k1pv7c73i7kbf5v1-libcxxabi-16.0.6
.drv' failed with exit code 1;
Hopefully rebasing on a newer staging commit fixes that?
|
I’m going to close this for now because it doesn’t seem necessary at the moment (see prior comments). I don’t really understand why GHC went from requiring patches to not, but it’s fine if that remains the case. |
Description of changes
When the stdenv is using a newer clang, it fails on non-x86_64 platforms due to an unused
popcount16function, which is treated as an error.This only affects GHC 9.4.x, which I assume is due to a quirk of its build system that GHC 8.8.x, 8.10.x, 9.2.x, and 9.6.x do not share.
Testing was done on x86_64-darwin and aarch64-darwin with #241692 applied.
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/)