clang_15: add nostdlibinc flag#217825
Conversation
The same patch applies to all Clang versions using it.
Port of 44165d3 ("llvmPackages_{14, git}.clang: add nostdlibinc flag") to Clang 15. It was originally thought this wasn't needed[1], but it is, to fix expressions like the following: with import ./. {}; llvmPackages_15.libcxxStdenv.mkDerivation { name = "libcxx-stdenv-c++-test"; dontUnpack = true; input = '' #include <cstdlib> int main() { std::abort(); return 0; } ''; passAsFile = [ "input" ]; installPhase = '' $CXX -c -o $out -x c++ $inputPath ''; } [1]: NixOS#194634 (comment)
rrbutani
left a comment
There was a problem hiding this comment.
Thanks for fixing this! I realized here that we actually do need this patch for llvmPackages_15.clang but hadn't gotten around to making the PR 🙂.
Good call on deduplicating the patch files, I'd missed that this one hadn't actually changed across LLVM 13+.
It'd be great to have a regression test for this somewhere but it's not super apparent to me where the right place would be (cc-wrapper tests (not yet enabled for llvmPackages_15)? an extra passthru test on libcxxStdenv?) and that doesn't need to block this PR anyways.
| @@ -1,18 +0,0 @@ | |||
| diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp | |||
There was a problem hiding this comment.
nit, feel free to ignore: I think it'd be useful to have a comment on this patch file giving a little context about why this is required; maybe an abbreviated version of what's in this comment, something like:
`--gcc-toolchain` causes `libstdc++` paths from `gcc` to be included, breaking `libc++`
`include_next` directives and we can't remove `--gcc-toolchain` when using `libc++`
because we may want to use `libgcc_s` with `libc++`; `--nostdlibinc` lets us retain the
`--gcc-toolchain` flag while keeping it from influencing include paths
Granted, I don't think we'll run into a situation like #194634 again for this patch but even so.
There was a problem hiding this comment.
I think this is adequately covered by my commit message, which I made quite thorough for exactly this reason. IMO it's more useful to have a commit that you can checkout and be guaranteed to reproduce to understand the failure, than a comment that might go out of date.
There was a problem hiding this comment.
👍 That's fair; it took me a little while to realize why the flag was actually needed but agreed that the test case is more valuable; once you've got it, it's pretty straightforward to figure the rest out.
Description of changes
Port of 44165d3 ("llvmPackages_{14, git}.clang: add nostdlibinc flag") to Clang 15. It was originally thought this wasn't needed, but it is, to fix expressions like the following:
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/)