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

cc-wrapper: add libcxxabi include flag for LLVM #255488

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

natto1784
Copy link
Contributor

Description of changes

Should fix #255487

Things done

Added -isystem ${libcxxabi}/include/c++/v1 to libcxx-cxxflags for clang

  • 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.11 Release Notes (or backporting 23.05 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.

# .../include/cxxabi.h:20:10: fatal error: '__cxxabi_config.h' file not found
# in libcxxStdenv
echo "checking whether cxxabi.h can be included... " >&2
$CXX -o include-cxxabi ${./include-cxxabi.cc}
Copy link
Member

Choose a reason for hiding this comment

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

This pr fixes this test for tests.cc-wrapper.llvmTests.llvmPackages_{15,16}.libcxx

# https://github.com/llvm/llvm-project/issues/55632
# ("16.0.3 libcxx, libcxxabi: circular build dependencies")
# Looks like the machinery changed in https://reviews.llvm.org/D120727.
"-I${lib.getDev targetLlvmLibraries.libcxx.cxxabi}/include/c++/v1"
Copy link
Member

Choose a reason for hiding this comment

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

removed old workaround

@Artturin Artturin changed the base branch from master to staging September 17, 2023 21:37
@Artturin
Copy link
Member

I have to consider whether we should do this or

install -m 644 ../../${pname}/include/__cxxabi_config.h "$dev/include" in libcxxabi postInstall, were already copying cxxabi.h in there

@@ -469,6 +469,7 @@ stdenv.mkDerivation {
''
+ optionalString (libcxx.isLLVM or false) ''
echo "-isystem ${lib.getDev libcxx}/include/c++/v1" >> $out/nix-support/libcxx-cxxflags
Copy link
Member

@Artturin Artturin Sep 17, 2023

Choose a reason for hiding this comment

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

Before llvm 15 __cxxabi_config.h was included in libcxx so this worked correctly

llvmPackages_13.libcxx.dev                        3,133 r /nix/store/5wfncnq3svyf54hfahi44smavyfbhq5b-libcxx-13.0.1-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_7.libcxx.dev                         2,278 r /nix/store/2i745a6pvd2qbi0lf9a61ia0svvg9q0b-libcxx-7.1.0-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_14.libcxx.dev                        3,142 r /nix/store/jri1wy9fxnnln4wv7dmzdhs9w5ll4jfa-libcxx-14.0.6-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_15.libcxxabi.dev                     3,266 r /nix/store/bbvhnvq66lr6yzqxqbzff2341h9xg332-libcxxabi-15.0.7-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_6.libcxx.dev                         2,032 r /nix/store/bpyj2sk6mh9vlli539jsr0w45rpig5pc-libcxx-6.0.1-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_5.libcxx.dev                         2,032 r /nix/store/fciabccmn17ijmfxxn50diin0dcjz7m6-libcxx-5.0.2-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_8.libcxx.dev                         2,278 r /nix/store/8sg9d3j38i3p13sg7pp4bc44szqakj7s-libcxx-8.0.1-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_9.libcxx.dev                         2,411 r /nix/store/1b5yn18w09n9r50i9162zvn3zq3jn98k-libcxx-9.0.1-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_16.libcxxabi.dev                     3,266 r /nix/store/02wpjmp2zjjxz13z7g599mniwi25zkcy-libcxxabi-16.0.6-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_10.libcxx.dev                        2,500 r /nix/store/j41yp16y29b7gw2bd5c1ljfyyykgsh12-libcxx-10.0.1-dev/include/c++/v1/__cxxabi_config.h
llvmPackages_12.libcxx.dev                        3,133 r /nix/store/fyr084bq8496099nllfjzapis7mxpj39-libcxx-12.0.1-dev/include/c++/v1/__cxxabi_config.h

and
cxxabi.h was in both libcxx and libcxxabi

llvmPackages_13.libcxx.dev                        7,215 r /nix/store/5wfncnq3svyf54hfahi44smavyfbhq5b-libcxx-13.0.1-dev/include/c++/v1/cxxabi.h
llvmPackages_7.libcxx.dev                         6,945 r /nix/store/2i745a6pvd2qbi0lf9a61ia0svvg9q0b-libcxx-7.1.0-dev/include/c++/v1/cxxabi.h
llvmPackages_14.libcxx.dev                        7,215 r /nix/store/jri1wy9fxnnln4wv7dmzdhs9w5ll4jfa-libcxx-14.0.6-dev/include/c++/v1/cxxabi.h
libcxxrt.out                                      9,040 r /nix/store/sdw3q6nzxgdh6n6jdnnsgm7jpg5wsp1v-libcxxrt-unstable-2022-08-08/include/cxxabi.h
llvmPackages_12.libcxxabi.dev                     7,215 r /nix/store/6lzf327lxdi743i8n8s3igq962jcp5r7-libcxxabi-12.0.1-dev/include/cxxabi.h
llvmPackages_15.libcxxabi.dev                     7,237 r /nix/store/bbvhnvq66lr6yzqxqbzff2341h9xg332-libcxxabi-15.0.7-dev/include/c++/v1/cxxabi.h
llvmPackages_15.libcxxabi.dev                     7,237 r /nix/store/bbvhnvq66lr6yzqxqbzff2341h9xg332-libcxxabi-15.0.7-dev/include/cxxabi.h
llvmPackages_5.libcxxabi.dev                      6,945 r /nix/store/qlzdwhhd0x9fda4mk1a83jmmxza4a64p-libcxxabi-5.0.2-dev/include/cxxabi.h
libcxxabi.dev                                     7,029 r /nix/store/h84lr9b4q34f37jvqzqkkf4gnrlrgahl-libcxxabi-11.1.0-dev/include/cxxabi.h
llvmPackages_14.libcxxabi.dev                     7,215 r /nix/store/zkfgj0hy0za0yhwnrfdndvzr301aj4pq-libcxxabi-14.0.6-dev/include/cxxabi.h
llvmPackages_6.libcxx.dev                         6,945 r /nix/store/bpyj2sk6mh9vlli539jsr0w45rpig5pc-libcxx-6.0.1-dev/include/c++/v1/cxxabi.h
llvmPackages_5.libcxx.dev                         6,945 r /nix/store/fciabccmn17ijmfxxn50diin0dcjz7m6-libcxx-5.0.2-dev/include/c++/v1/cxxabi.h
llvmPackages_8.libcxx.dev                         6,987 r /nix/store/8sg9d3j38i3p13sg7pp4bc44szqakj7s-libcxx-8.0.1-dev/include/c++/v1/cxxabi.h
llvmPackages_9.libcxx.dev                         7,023 r /nix/store/1b5yn18w09n9r50i9162zvn3zq3jn98k-libcxx-9.0.1-dev/include/c++/v1/cxxabi.h
llvmPackages_16.libcxxabi.dev                     7,237 r /nix/store/02wpjmp2zjjxz13z7g599mniwi25zkcy-libcxxabi-16.0.6-dev/include/c++/v1/cxxabi.h
llvmPackages_16.libcxxabi.dev                     7,237 r /nix/store/02wpjmp2zjjxz13z7g599mniwi25zkcy-libcxxabi-16.0.6-dev/include/cxxabi.h
llvmPackages_10.libcxxabi.dev                     7,029 r /nix/store/pwx93kc21scj2ikif98d9zzyzaicag8x-libcxxabi-10.0.1-dev/include/cxxabi.h
llvmPackages_7.libcxxabi.dev                      6,945 r /nix/store/wx2xdg98ddhrs5iif85w8dgmskgkmjv2-libcxxabi-7.1.0-dev/include/cxxabi.h
llvmPackages_9.libcxxabi.dev                      7,023 r /nix/store/lhjbqxfqqckm3x2ci9v7vgblmw6jr2jc-libcxxabi-9.0.1-dev/include/cxxabi.h
llvmPackages_6.libcxxabi.dev                      6,945 r /nix/store/lc41k1nfaslcp8gbgl948ksx52n19j20-libcxxabi-6.0.1-dev/include/cxxabi.h
llvmPackages_8.libcxxabi.dev                      6,987 r /nix/store/khf9r8s7llkmpsxza19pqavr26j3w6lb-libcxxabi-8.0.1-dev/include/cxxabi.h
llvmPackages_10.libcxx.dev                        7,029 r /nix/store/j41yp16y29b7gw2bd5c1ljfyyykgsh12-libcxx-10.0.1-dev/include/c++/v1/cxxabi.h
llvmPackages_12.libcxx.dev                        7,215 r /nix/store/fyr084bq8496099nllfjzapis7mxpj39-libcxx-12.0.1-dev/include/c++/v1/cxxabi.h

because we were copying it manually to libcxx.dev/include

@Artturin
Copy link
Member

I have to consider whether we should do this or

install -m 644 ../../${pname}/include/__cxxabi_config.h "$dev/include" in libcxxabi postInstall, were already copying cxxabi.h in there

basically if we do this then we don't have to special case cxxabi in the wrapper and can benefit from the hook

if [ -d "$1/include" ]; then

@Artturin
Copy link
Member

Let's do this

TODO remove now should be unnecessary postInstalls from all libcxxabi's

@Artturin Artturin marked this pull request as ready for review September 17, 2023 22:50
@Artturin
Copy link
Member

Artturin commented Sep 17, 2023

@natto1784 do you want to re-sign the first commit before merge.

CC @pwaller

Copy link
Member

@Artturin Artturin left a comment

Choose a reason for hiding this comment

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

This restores llvm <15 behaviour

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 17, 2023
@natto1784
Copy link
Contributor Author

Let's do this

TODO remove now should be unnecessary postInstalls from all libcxxabi's

Assuming this is redundant

 install -m 644 ../../${pname}/include/${if stdenv.isDarwin then "*" else "cxxabi.h"} "$dev/include"

I am curious as to why is everything installed from the $src itself for darwin but not other platforms

@natto1784 do you want to re-sign the first commit before merge.

CC @pwaller

Oh yeah sure

natto1784 and others added 2 commits September 18, 2023 06:43
Removed workaround from llvm 16.

Fixes including cxxabi.h on llvm >=15 libcxxStdenv.

```c
int main() {}
```

```
/nix/store/qwnvng0cbyx0bijm654jpmpl0516hfhx-libcxxabi-15.0.7-dev/include/cxxabi.h:20:10: fatal error: '__cxxabi_config.h' file not found
```

Before llvm 15 this used to work because `libcxx` copied the headers
from `cxxabi` to it's own `include`, which was then picked up by the
line above this one

Alternative fix would be to copy all files from `${cxxabi.dev}/include/c++/v1` to `${cxxabi.dev}/include` so the cc-wrapper setup hook would pick them up, but that would depend on in cxxabi being in buildInputs.

Signed-off-by: Amneesh Singh <[email protected]>
`#include <cxxabi.h>`

`/nix/store/02wpjmp2zjjxz13z7g599mniwi25zkcy-libcxxabi-16.0.6-dev/include/cxxabi.h:20:10: fatal error: '__cxxabi_config.h' file not found`
@Artturin
Copy link
Member

Let's do this
TODO remove now should be unnecessary postInstalls from all libcxxabi's

Assuming this is redundant

 install -m 644 ../../${pname}/include/${if stdenv.isDarwin then "*" else "cxxabi.h"} "$dev/include"

I am curious as to why is everything installed from the $src itself for darwin but not other platforms

I'm going to leave that for later because removing it is not necessary in this PR.

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 18, 2023
@Artturin Artturin merged commit 6f27ba8 into NixOS:staging Sep 20, 2023
9 checks passed
@wegank wegank mentioned this pull request Dec 7, 2023
12 tasks
@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
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 101-500
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'__cxxabi_config.h' file not found with clang-wrapper
4 participants