rocmPackages.llvm.clang: remove -nostdlibinc flag #370180
Conversation
collares
changed the title
github-actions
bot
added
10.rebuild-darwin: 0
collares
force-pushed
the
rocm-fix
branch
2 times, most recently
from
505011c to
b593bd3
Compare
|
rocm-device-libs fails to build with some errors which look related to libcxx. I'm not sure this PR is correct anymore, since it looks like rocm is supposed to be built with libstdc++ instead of libcxx and that's not currently happening. Suggestions are welcome. Currently testing just this, which will probably use libstdc++ instead: diff --git a/pkgs/development/rocm-modules/6/llvm/stage-2/rstdenv.nix b/pkgs/development/rocm-modules/6/llvm/stage-2/rstdenv.nix
index f83abe36cc2e..e44d0689363b 100644
--- a/pkgs/development/rocm-modules/6/llvm/stage-2/rstdenv.nix
+++ b/pkgs/development/rocm-modules/6/llvm/stage-2/rstdenv.nix
@@ -11,13 +11,13 @@
overrideCC stdenv (wrapCCWith rec {
inherit bintools;
- libcxx = runtimes;
cc = clang-unwrapped;
gccForLibs = stdenv.cc.cc;
extraPackages = [
llvm
lld
+ runtimes
];
nixSupport.cc-cflags = [Edit: I updated the PR to use a hackier but more straightforward strategy. |
|
I confirm that this definitely seems messed up but sadly don’t understand ROCm enough to know what the correct solution is. By “built with |
|
@emilazy @GZGavinZhao I've updated the PR. Do you think this hack is an acceptable workaround to fix the build while #370435 is being prepared? This is the minimum change I could think of to unbreak the build. |
|
So I guess what’s going on here is that the ROCm LLVM never bundled the patch from the main LLVM derivation to add |
|
Yes, exactly, that's most of the issue. The other part is that ROCm wanted to pin GCC 12, but forgot to set |
|
Right. So this seems okay for the short term, but it’d be definitely nicer to do it in the wrapper code directly rather than adding it in and then removing it later. |
collares
changed the title
|
To be clear, doing this in the wrapper doesn’t involve reusing the entire LLVM derivation. It’s just the |
|
But wouldn't changing |
|
No, because builds are based on the derivations produced after evaluation. Since this will only affect the evaluation results when targeting ROCm, there will be no more rebuilds than expected, and we can do it now. |
|
OK, made the change, but I still have to build it. If you OK the current state of the PR, I can merge it once the build is done. |
| @@ -639,7 +639,7 @@ stdenvNoCC.mkDerivation { | |||
| # no `/usr/include`, there’s essentially no risk to dropping | |||
| # the flag there. See discussion in NixOS/nixpkgs#191152. | |||
| # | |||
| + optionalString ((cc.isClang or false) && !targetPlatform.isDarwin) '' | |||
| + optionalString ((cc.isClang or false) && !(cc.isROCm or false) && !targetPlatform.isDarwin) '' | |||
There was a problem hiding this comment.
Could this be e.g. !targetPlatform.isROCm? Or is ROCm not actually using the proper cross‐compilation machinery?
LGTM as a temporary fix, anyway.
There was a problem hiding this comment.
Offload code is treated as an auxiliary arch and doesn't fit into the current cross machinery.
eg:
host, target = x86-64
auxiliary-targets = [gfx908, gfx90a, ...]
auxiliary target code objects are embedded in the same output library/bin in a separate section.
There was a problem hiding this comment.
I see; sounds complicated :)
This seems fine for now.
There was a problem hiding this comment.
From what I can tell it isn't using the cross-compilation machinery, but to be honest I know nothing about ROCm (I'm just here because Steam broke). Maybe one of the maintainers can chime in (Edit: I hadn't seen the above comments when I wrote this comment, sorry!).
Thanks for the review!
nix-owners
bot
requested review from
acowley,
Flakebi,
lovesegfault and
mschwaig
|
The issue still persists for me: |
|
@Tjorbenn What nixpkgs commit are you testing, and what command are you using to test it? Thanks! Edit: If you're using nixos-unstable, you can use https://nixpk.gs/pr-tracker.html?pr=370180 to check if the channel has advanced to include the fix. |
Yeah my bad, nevermind. I thought that if it was merged onto the master branch, it was available in nixpkgs unstable. |
As reported in #368672,
rocmPackages.llvm.libcfails to build. The proximate cause is #356162, but I think it just revealed a problem with the rocm package. More specifically, there seems to be something fishy innixpkgs/pkgs/development/rocm-modules/6/llvm/stage-2/rstdenv.nix
Line 14 in 4f0dadb
I think
libcxx-cxxflagsonly gets populated in the wrapper iflibcxxis null orlib.isLLVMis true...nixpkgs/pkgs/build-support/cc-wrapper/default.nix
Lines 588 to 599 in 4f0dadb
...but
runtimesis neither:nixpkgs/pkgs/development/rocm-modules/6/llvm/stage-1/runtimes.nix
Lines 1 to 32 in 4f0dadb
Since nothing ROCm-related builds at the moment, this PR adds a minimal workaround, namely hackily removing
-nostdlibincfromnix-support/cc-cflags. Turns out the gcc 14 update also caused a different libstdc++ version to be used in ROCm's wrapped Clang, so we now also passgccForLibsexplicitly.cc @emilazy @LunNova (because of #367695) @NixOS/rocm-maintainers
Fixes #368672, fixes #369433
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.