Prefer libc++ headers in the -isysroot over the toolchain

[ldionne]

Summary

Currently, Clang looks for libc++ headers alongside the installation directory of Clang, and it then looks for libc++ headers in the sysroot. This patch reverses the order so that headers are searched for in the following order:

1. <sysroot>/usr/include/c++/v1
2. <installdir>/bin/../include/c++/v1

The benefit of doing this is that a user-installed libc++ can be picked up when providing a custom sysroot, which doesn't work properly right now.

[dexonsmith]

Furthermore, instead of always adding both search paths, we find
the first search path that exists and only add that one.

Huh, I'm surprised this wasn't already the behaviour. This seems like a good patch to split out and commit first, since in the (somewhat unlikely) case this causes a problem for someone it would be good for them to be able to bisect this separately the other part.

[dexonsmith]

Note that I don't see any issues besides splitting the patch and dealing with the clang-format linter problems.

[ldionne]

Split the patch in two

[rsmith]

This seems like it would break the opposite situation: a user places a custom clang and the corresponding libc++ somewhere (perhaps in $HOME, or perhaps they run Clang from the build tree). Right now we use the custom clang and its corresponding libc++, but with this change, I would expect we'd instead pick up the SDK version of libc++. Is that what we want?

[teemperor]

On macOS, LLDB's tests pass -isysroot /path/to/SDK to Clang but expect that Clang always prefers the the libc++ headers that are in the Clang build directory. So if we change this behaviour then we should have a way to keep the old behaviour around.

[dexonsmith]

Hmm, that's right, to support user-provided libc++ headers it's simpler if we prefer the toolchain / next-to-clang.

The reason we need to invert it is transitional. The status quo is that we have no headers in the SDKs (either internally or publicly). We want to move the headers from the toolchain to the SDK. For [reasons], in some internal contexts we'll have headers both places for a small number of years, where the SDK headers should be preferred if both exist.

Some ideas:

• Add a command-line option to decide whether to check the SDK or next-to-clang first. Maybe we can restrict it to a -cc1 option.
• Make the default behaviour configurable, and potentially use a different default in the Apple CMake cache.

WDYT?

[dexonsmith]

@rsmith , @teemperor , @ldionne , thoughts on the above?

[teemperor]

From the LLDB side we can just change our test setup to explicitly add an include the libc++ in our build tree (and have -nostdinc to avoid the libc++ in the SDK). So it wouldn't be a problem for us. I can make a review for that once this gets accepted.

[ldionne]

Revisiting this, I don't think we need to move forward with this. Let's just carry our downstream patch until we don't need it anymore (which should be soon ish), and leave upstream Clang as it is (I think the current behavior makes the most sense). I'm abandoning this - I'll reopen if someone thinks that having Clang and AppleClang behave differently for a little while is harmful, but I don't think it is.

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-toolchain-root/bin/clang

 This file is a placeholder to keep its parent directory in git.

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-toolchain-root/include/c++/v1/mock_vector

 class vector {};
 typedef vector* vector_ptr;

clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-mac-libcxx.cpp

-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure clang-tidy emulates this properly.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our clang-tidy emulates
+// this properly.
 //
 // RUN: rm -rf %t
 // RUN: mkdir %t
 //
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
 //
-// Pretend clang is installed beside the mock library that we provided.
-// RUN: echo '[{"directory":"%t","command":"%t/mock-libcxx/bin/clang++ -stdlib=libc++ -std=c++11 -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// Install a mock sysroot that doesn't contain anything, to make sure we
+// prefer the libc++ headers in the toolchain.
+// RUN: cp -r %S/Inputs/empty-sysroot %t/empty-sysroot
+//
+// RUN: echo '[{"directory":"%t","command":"%t/mock-toolchain-root/bin/clang -isysroot %t/empty-sysroot -stdlib=libc++ -std=c++11 -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
 // RUN: cp "%s" "%t/test.cpp"
 // RUN: clang-tidy -header-filter='.*' -system-headers -checks='-*,modernize-use-using'  "%t/test.cpp" | FileCheck %s
 // CHECK: mock_vector:{{[0-9]+}}:{{[0-9]+}}: warning: use 'using' instead of 'typedef'
 
 #include <mock_vector>
 typedef vector* vec_ptr;

clang/lib/Driver/ToolChains/Darwin.cpp

if (DriverArgs.hasArg(options::OPT_nostdlibinc) ||
       DriverArgs.hasArg(options::OPT_nostdincxx))
     return;
 
   llvm::StringRef Sysroot = GetHeaderSysroot(DriverArgs);
 
   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
+    // 1. In a SDK (or a custom sysroot) in <sysroot>/usr/include/c++/v1
+    // 2. Alongside the compiler in         <install>/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
     // include_next could break).
 
     // Check for (1)
+    llvm::SmallString<128> SysrootUsr = Sysroot;
+    llvm::sys::path::append(SysrootUsr, "usr", "include", "c++", "v1");
+    if (getVFS().exists(SysrootUsr)) {
+      addSystemInclude(DriverArgs, CC1Args, SysrootUsr);
+      return;
+    } else if (DriverArgs.hasArg(options::OPT_v)) {
+      llvm::errs() << "ignoring nonexistent directory \"" << SysrootUsr
+                   << "\"\n";
+    }
+
+    // Otherwise, check for (2)
     // Get from '<install>/bin' to '<install>/include/c++/v1'.
     // Note that InstallBin can be relative, so we use '..' instead of
     // parent_path.
     llvm::SmallString<128> InstallBin =
         llvm::StringRef(getDriver().getInstalledDir()); // <install>/bin
     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 (2)
-    llvm::SmallString<128> SysrootUsr = Sysroot;
-    llvm::sys::path::append(SysrootUsr, "usr", "include", "c++", "v1");
-    if (getVFS().exists(SysrootUsr)) {
-      addSystemInclude(DriverArgs, CC1Args, SysrootUsr);
-      return;
-    } else if (DriverArgs.hasArg(options::OPT_v)) {
-      llvm::errs() << "ignoring nonexistent directory \"" << SysrootUsr
-                   << "\"\n";
-    }
-
     // Otherwise, don't add any path.
     break;
   }
 
   case ToolChain::CST_Libstdcxx:
     llvm::SmallString<128> UsrIncludeCxx = Sysroot;
     llvm::sys::path::append(UsrIncludeCxx, "usr", "include", "c++");
 

clang/test/Driver/darwin-header-search-libcxx.cpp

 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain_no_libcxx \
 // RUN:               --check-prefix=CHECK-LIBCXX-SYSROOT-1 %s
 // CHECK-LIBCXX-SYSROOT-1: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
 // CHECK-LIBCXX-SYSROOT-1: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 // CHECK-LIBCXX-SYSROOT-1-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 
 // Check with both headers in the sysroot and headers alongside the installation
-// (the headers in the toolchain should be preferred over the <sysroot> headers).
+// (the headers in <sysroot> should be preferred over the toolchain headers).
 // Ensure that both -isysroot and --sysroot work, and that isysroot has precedence
 // over --sysroot.
 //
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
 // RUN:     -target x86_64-apple-darwin \
 // RUN:     -stdlib=libc++ \
 // RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \

 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:     -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
 // RUN:     --sysroot %S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
 // RUN:               --check-prefix=CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1 %s
 //
 // CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
-// CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 
 // Make sure that using -nostdinc does not drop any C++ library include path.
 // This behavior is strange, but it is compatible with the legacy CC1 behavior.
 //
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
 // RUN:     -target x86_64-apple-darwin16 \
 // RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:     -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
 // RUN:     -stdlib=platform \
 // RUN:     -nostdinc \
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
 // RUN:               --check-prefix=CHECK-LIBCXX-NOSTDINC-1 %s
 // CHECK-LIBCXX-NOSTDINC-1: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// CHECK-LIBCXX-NOSTDINC-1-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
-// CHECK-LIBCXX-NOSTDINC-1: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
+// CHECK-LIBCXX-NOSTDINC-1-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
+// CHECK-LIBCXX-NOSTDINC-1: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
 // RUN:     -target x86_64-apple-darwin16 \
 // RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:     -isysroot %S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:     -stdlib=platform \
 // RUN:     -nostdinc \

 // CHECK-LIBCXX-NOSTDLIBINC-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-NOSTDLIBINC-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 
 // Make sure we explain that we considered a path but didn't add it when it
 // doesn't exist.
 //
 // RUN: %clang -no-canonical-prefixes %s -fsyntax-only -v 2>&1 \
 // RUN:     -target x86_64-apple-darwin \
-// RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain_no_libcxx/usr/bin \
-// RUN:     -isysroot %S/Inputs/basic_darwin_sdk \
-// RUN:   | FileCheck -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain_no_libcxx \
-// RUN:               --check-prefix=CHECK-LIBCXX-MISSING-TOOLCHAIN %s
-// CHECK-LIBCXX-MISSING-TOOLCHAIN: ignoring nonexistent directory "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
+// RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
+// RUN:     -isysroot %S/Inputs/basic_darwin_sdk_no_libcxx \
+// RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_no_libcxx \
+// RUN:               --check-prefix=CHECK-LIBCXX-MISSING-SYSROOT %s
+// CHECK-LIBCXX-MISSING-SYSROOT: ignoring nonexistent directory "[[SYSROOT]]/usr/include/c++/v1"
 //
 // RUN: %clang -no-canonical-prefixes %s -fsyntax-only -v 2>&1 \
 // RUN:     -target x86_64-apple-darwin \
 // RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain_no_libcxx/usr/bin \
 // RUN:     -isysroot %S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain_no_libcxx \
 // RUN:               --check-prefix=CHECK-LIBCXX-MISSING-BOTH %s
-// CHECK-LIBCXX-MISSING-BOTH: ignoring nonexistent directory "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-MISSING-BOTH: ignoring nonexistent directory "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-LIBCXX-MISSING-BOTH: ignoring nonexistent directory "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"

clang/test/Tooling/Inputs/mock-toolchain-root/bin/clang

 This file is a placeholder to keep its parent directory in git.

clang/test/Tooling/Inputs/mock-toolchain-root/include/c++/v1/mock_vector

 class vector {};

clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp

-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure our libTooling-based tools emulate this properly.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our libTooling-based tools
+// emulate this properly.
 //
 // RUN: rm -rf %t
 // RUN: mkdir %t
 //
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
 //
-// Pretend clang is installed beside the mock library that we provided.
-// RUN: echo '[{"directory":"%t","command":"%t/mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// Install a mock sysroot that doesn't contain anything, to make sure we
+// prefer the libc++ headers in the toolchain.
+// RUN: cp -r %S/Inputs/empty-sysroot %t/empty-sysroot
+//
+// RUN: echo '[{"directory":"%t","command":"%t/mock-toolchain-root/bin/clang -isysroot %t/empty-sysroot -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
 // RUN: cp "%s" "%t/test.cpp"
+//
 // clang-check will produce an error code if the mock library is not found.
 // RUN: clang-check -p "%t" "%t/test.cpp"
 
 #include <mock_vector>
 vector v;

clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp

-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure our libTooling-based tools emulate this properly with
-// fixed compilation database.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our libTooling-based tools
+// emulate this properly with a fixed compilation database.
 //
 // RUN: rm -rf %t
 // RUN: mkdir %t
 //
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
 //
-// RUN: cp clang-check %t/mock-libcxx/bin/
+// Install a mock sysroot that doesn't contain anything, to make sure we
+// prefer the libc++ headers in the toolchain.
+// RUN: cp -r %S/Inputs/empty-sysroot %t/empty-sysroot
+//
+// RUN: cp clang-check %t/mock-toolchain-root/bin/
 // RUN: cp %s %t/test.cpp
-// RUN: "%t/mock-libcxx/bin/clang-check" -p %t %t/test.cpp -- \
+// RUN: "%t/mock-toolchain-root/bin/clang-check" -p %t %t/test.cpp -- \
 // RUN:     -stdlib=libc++ -target x86_64-apple-darwin \
-// RUN:     -ccc-install-dir %t/mock-libcxx/bin
+// RUN:     -isysroot %t/empty-sysroot \
+// RUN:     -ccc-install-dir %t/mock-toolchain-root/bin
 //
 // ^ -ccc-install-dir passed to unbreak tests on *BSD where
 //   getMainExecutable() relies on real argv[0] being passed
 //
 // UNSUPPORTED: enable_shared
 #include <mock_vector>
 vector v;

clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp

-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure our libTooling-based tools emulate this properly.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our libTooling-based tools
+// emulate this properly.
 //
 // RUN: rm -rf %t
 // RUN: mkdir %t
 //
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
 //
-// Pretend clang is installed beside the mock library that we provided.
-// RUN: echo '[{"directory":"%t","command":"mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// Install a mock sysroot that doesn't contain anything, to make sure we
+// prefer the libc++ headers in the toolchain.
+// RUN: cp -r %S/Inputs/empty-sysroot %t/empty-sysroot
+//
+// RUN: echo '[{"directory":"%t","command":"mock-toolchain-root/bin/clang -isysroot %t/empty-sysroot -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
 // RUN: cp "%s" "%t/test.cpp"
+//
 // clang-check will produce an error code if the mock library is not found.
 // RUN: clang-check -p "%t" "%t/test.cpp"
 
 #include <mock_vector>
 vector v;