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.

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

clang_15: add nostdlibinc flag #217825

Merged
merged 2 commits into from
Feb 26, 2023

Conversation

alyssais
Copy link
Member

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:

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
   '';
}
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.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.

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)
Copy link
Contributor

@rrbutani rrbutani left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 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.

@alyssais alyssais merged commit 1e26d33 into NixOS:staging Feb 26, 2023
@alyssais alyssais deleted the llvmPackages_15.libcxxStdenv branch February 26, 2023 13:40
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants