Skip to content

[clang][driver] Add \<executable\>/../include/c++/v1 to include path on Darwin #70817

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.

Sign up for GitHub

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

Merged
merged 18 commits into from
Nov 20, 2023
Merged

[clang][driver] Add \<executable\>/../include/c++/v1 to include path on Darwin #70817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7fbc229
Add \<executable\>/../include/c++/v1 to include path
ilg-ul Oct 31, 2023
b2f287c
Darwin.cpp: update comments
ilg-ul Nov 10, 2023
31b7482
Darwin.cpp: construct StringRef from string
ilg-ul Nov 10, 2023
6db76fe
Darwin.cpp: reformat comments
ilg-ul Nov 10, 2023
04b6913
add test
ilg-ul Nov 12, 2023
fc1108e
fix test
ilg-ul Nov 12, 2023
e62ac5e
fix test
ilg-ul Nov 12, 2023
e12726f
fix test
ilg-ul Nov 12, 2023
ec4b058
fix test
ilg-ul Nov 12, 2023
8b5eb52
fix test
ilg-ul Nov 13, 2023
7da33d4
test -v
ilg-ul Nov 13, 2023
231bb49
more debug
ilg-ul Nov 13, 2023
ea5c34c
test tweaks
ilg-ul Nov 13, 2023
6bfa325
Darwin.cpp clang-format
ilg-ul Nov 13, 2023
198ce34
test mkdir in executable folder
ilg-ul Nov 13, 2023
d171069
cleanups
ilg-ul Nov 13, 2023
132a328
test create local install
ilg-ul Nov 13, 2023
0acce2a
update tests
ilg-ul Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions clang/lib/Driver/ToolChains/Darwin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2470,14 +2470,19 @@ void DarwinClang::AddClangCXXStdlibIncludeArgs(

switch (GetCXXStdlibType(DriverArgs)) {
case ToolChain::CST_Libcxx: {
// On Darwin, libc++ can be installed in one of the following two places:
// 1. Alongside the compiler in <install>/include/c++/v1
// 2. In a SDK (or a custom sysroot) in <sysroot>/usr/include/c++/v1
// On Darwin, libc++ can be installed in one of the following places:
// 1. Alongside the compiler in <install>/include/c++/v1
// 2. Alongside the compiler in <clang-executable-folder>/../include/c++/v1
// 3. In a SDK (or a custom sysroot) in <sysroot>/usr/include/c++/v1
//
// The precendence of paths is as listed above, i.e. we take the first path
// that exists. Also note that we never include libc++ twice -- we take the
// first path that exists and don't send the other paths to CC1 (otherwise
// The precedence of paths is as listed above, i.e. we take the first path
// that exists. Note that we never include libc++ twice -- we take the first
// path that exists and don't send the other paths to CC1 (otherwise
// include_next could break).
//
// Also note that in most cases, (1) and (2) are exactly the same path.
// Those two paths will differ only when the `clang` program being run
// is actually a symlink to the real executable.

// Check for (1)
// Get from '<install>/bin' to '<install>/include/c++/v1'.
Expand All @@ -2494,7 +2499,20 @@ void DarwinClang::AddClangCXXStdlibIncludeArgs(
<< "\"\n";
}

// Otherwise, check for (2)
// (2) Check for the folder where the executable is located, if different.
if (getDriver().getInstalledDir() != getDriver().Dir) {
InstallBin = llvm::StringRef(getDriver().Dir);
llvm::sys::path::append(InstallBin, "..", "include", "c++", "v1");
if (getVFS().exists(InstallBin)) {
addSystemInclude(DriverArgs, CC1Args, InstallBin);
return;
} else if (DriverArgs.hasArg(options::OPT_v)) {
llvm::errs() << "ignoring nonexistent directory \"" << InstallBin
<< "\"\n";
}
}

// Otherwise, check for (3)
llvm::SmallString<128> SysrootUsr = Sysroot;
llvm::sys::path::append(SysrootUsr, "usr", "include", "c++", "v1");
if (getVFS().exists(SysrootUsr)) {
Expand Down
57 changes: 57 additions & 0 deletions clang/test/Driver/darwin-header-search-libcxx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,3 +172,60 @@
// RUN: --check-prefix=CHECK-LIBCXX-STDLIB-UNSPECIFIED %s
// CHECK-LIBCXX-STDLIB-UNSPECIFIED: "-cc1"
// CHECK-LIBCXX-STDLIB-UNSPECIFIED: "-internal-isystem" "[[SYSROOT]]/usr/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.

Can you try to integrate your test to the existing ones a bit more closely? They are not really specific to xpm/npm, so it makes sense to support them in a platform agnostic way.

For example, we need to add a test where we set up a symlinked Clang and then run with a toolchain that has headers in it, and confirm that this takes precedence over the ones in the symlink directory. In other words, we now have (1), (2) and (3) places where we look for headers in. Previously, we pretty exhaustively checked the combinations for the two locations we checked -- now we should at least try to check:

  • Headers in (1) and in (2) -> (1) is preferred over (2)
  • Headers in (2) and in (3) -> (2) is preferred over (3) -- you already check that with your current test
  • Headers in (2) and nowhere else -> (2) is used

Also, we should make the comments in the tests platform-agnostic. We also probably want to insert the tests in the right place in the file and I suspect that using (1) (2) and (3) terminology to explain what we're testing would help.

Copy link
Member

Choose a reason for hiding this comment

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

BTW this looks good to me apart from these test improvements.

// On Darwin, libc++ can be installed in one of the following places:
// 1. Alongside the compiler in <install>/include/c++/v1
// 2. Alongside the compiler in <clang-executable-folder>/../include/c++/v1
// 3. In a SDK (or a custom sysroot) in <sysroot>/usr/include/c++/v1

// The build folders do not have an `include/c++/v1`; create a new
// local folder hierarchy that meets this requirement.
// Note: this might not work with weird RPATH configurations.
// RUN: rm -rf %t/install
// RUN: mkdir -pv %t/install/bin
// RUN: cp %clang %t/install/bin/clang
// RUN: mkdir -pv %t/install/include/c++/v1

// Headers in (1) and in (2) -> (1) is preferred over (2)
// RUN: rm -rf %t/symlinked1
// RUN: mkdir -pv %t/symlinked1/bin
// RUN: ln -svf %t/install/bin/clang %t/symlinked1/bin/clang
// RUN: mkdir -pv %t/symlinked1/include/c++/v1

// RUN: %t/symlinked1/bin/clang -### %s -fsyntax-only 2>&1 \
// RUN: --target=x86_64-apple-darwin \
// RUN: -stdlib=libc++ \
// RUN: -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
// RUN: | FileCheck -DSYMLINKED=%t/symlinked1 \
// RUN: -DTOOLCHAIN=%t/install \
// RUN: -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
// RUN: --check-prefix=CHECK-SYMLINKED-INCLUDE-CXX-V1 %s
// CHECK-SYMLINKED-INCLUDE-CXX-V1: "-internal-isystem" "[[SYMLINKED]]/bin/../include/c++/v1"
// CHECK-SYMLINKED-INCLUDE-CXX-V1-NOT: "-internal-isystem" "[[TOOLCHAIN]]/bin/../include/c++/v1"
// CHECK-SYMLINKED-INCLUDE-CXX-V1-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"

// Headers in (2) and in (3) -> (2) is preferred over (3)
// RUN: rm -rf %t/symlinked2
// RUN: mkdir -pv %t/symlinked2/bin
// RUN: ln -svf %t/install/bin/clang %t/symlinked2/bin/clang

// RUN: %t/symlinked2/bin/clang -### %s -fsyntax-only 2>&1 \
// RUN: --target=x86_64-apple-darwin \
// RUN: -stdlib=libc++ \
// RUN: -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
// RUN: | FileCheck -DTOOLCHAIN=%t/install \
// RUN: -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
// RUN: --check-prefix=CHECK-TOOLCHAIN-INCLUDE-CXX-V1 %s
// CHECK-TOOLCHAIN-INCLUDE-CXX-V1: "-internal-isystem" "[[TOOLCHAIN]]/bin/../include/c++/v1"
// CHECK-TOOLCHAIN-INCLUDE-CXX-V1-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"

// Headers in (2) and nowhere else -> (2) is used
// RUN: %t/symlinked2/bin/clang -### %s -fsyntax-only 2>&1 \
// RUN: --target=x86_64-apple-darwin \
// RUN: -stdlib=libc++ \
// RUN: -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
// RUN: | FileCheck -DTOOLCHAIN=%t/install \
// RUN: -DSYSROOT=%S/Inputs/basic_darwin_sdk_no_libcxx \
// RUN: --check-prefix=CHECK-TOOLCHAIN-NO-SYSROOT %s
// CHECK-TOOLCHAIN-NO-SYSROOT: "-internal-isystem" "[[TOOLCHAIN]]/bin/../include/c++/v1"