llvmPackages_*.clangUseLLVM: add libunwind to lib search path (#201591) #220520
Conversation
|
I guess that brings us to the old question of whether |
|
Stale but related #207325. |
There was a problem hiding this comment.
I think the change is needed to make nix shell nixpkgs#pkgsLLVM.stdenv.cc be a functioning compiler.
On master prior to this patch, clangUseLLVM gives you a functioning compiler if the stdenv is used to build software, but then if you try to use the compiler outside of the stdenv it fails. The reason is that the binutils setup hook propagates libunwind (and other runtimes) into the NIX_LDFLAGS when you're in the stdenv because these packages are specified in extraPackages.
Since -lunwind and friends are a part of the compiler wrapper, it makes sense to me that the -L flags should also be a part of the wrapper, as you have done here.
Just one comment that they'll need conditionalising, I think -L/path/to/unwind should only be added if -lunwind is being added, it might be that the package won't build outside of those scenarios for example.
| @@ -155,6 +155,7 @@ let | |||
| extraBuildCommands = '' | |||
| echo "-rtlib=compiler-rt -Wno-unused-command-line-argument" >> $out/nix-support/cc-cflags | |||
| echo "-B${targetLlvmLibraries.compiler-rt}/lib" >> $out/nix-support/cc-cflags | |||
| echo "-L${targetLlvmLibraries.libunwind}/lib" >> $out/nix-support/cc-ldflags | |||
There was a problem hiding this comment.
Given that the library is conditional on !isWasm below and targetPlatform.useLLVM, this should probably be along side that.
There was a problem hiding this comment.
clang links against libunwind when -unwindlib=libunwind is set. Hence if conditioning on targetPlatform.useLLVM, llvmPackages.clangUseLLVM still cannot find libunwind.
There was a problem hiding this comment.
It's definitely curious that we gate -lunwind on targetPlatform.useLLVM but not --unwindlib=libunwind.
Tracing back to where the -lunwind was added doesn't reveal any obvious reason for the discrepancy and I'm actually not clear on why the -lunwind was needed in the first place (my understanding is that it's implied). Perhaps there were scenarios where the linker was being invoked directly with the expectation that libunwind would be pulled in?
Regardless, doesn't need to block this PR. Adding this -L in shouldn't hurt.
|
cc @reckenrode are you aware of whether this could cause problems on darwin? |
I shouldn’t think so. The only risk would be if it could cause an infinite recursion, but the ofborg eval check should catch that. |
SharzyL
force-pushed
the
llvm_add_libunwind
branch
from
f27b8c2
to
fba2fbb
Compare
|
Rebased the commit and added conditioning on |
There was a problem hiding this comment.
Seems reasonable to me, unfortunately I don't have commit rights. @reckenrode ? @RaitoBezarius ?
There was a problem hiding this comment.
I think @sternenseemann's question is the right one. It would be nicer to have something other than clangUseLLVM intended for use outside of stdenv but in the interim I think it makes sense to merge this fix.
It would be nice if we could have a test guarding against regressions but that doesn't need to block this PR.
@SharzyL thanks for your patience.
rrbutani
added
12.approvals: 2
|
Apologies for the delay, @SharzyL can you rebase for the conflicts and after that we go for the merge IMHO. |
SharzyL
force-pushed
the
llvm_add_libunwind
branch
from
fba2fbb
to
810cef7
Compare
|
Rebase is complete |
|
This probably should not have targeted staging given the low amount of rebuilds, but well, it can wait staging :). |
|
Is the exclusion of LLVM 17 intended in this PR? |
|
Probably a omittance during rebase |
llvmPackages_17.clangUseLLVM: apply omitted #220520
rrbutani
added
the
6.topic: llvm/clang
Description of changes
resolves #201591
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/)