[clang] Match -isysroot behaviour with system compiler on Darwin

[DmitryPolukhin]

Summary

See discussion in https://reviews.llvm.org/D89001

I updated test to match actual behavior of
/Applications/Xcode_14.3.1_14E300b.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
after that modified upstream to match the test.

[dexonsmith]

This looks correct to me, but I'd rather have someone at Apple confirm. @ldionne (or @arphaman or @jroelofs), can you review and/or help to find the right person?

[DmitryPolukhin]

Update comments

[DmitryPolukhin]

@ldionne, @arphaman and @jroelofs, friendly ping, could you please take a look?

[jroelofs]

LGTM, thank you!

[PiotrZSL]

I don't know how, but somehow this change breaks clang-tidy/infrastructure/clang-tidy-mac-libcxx.cpp test.

[jroelofs]

In D157283#4611447, @PiotrZSL wrote:

I don't know how, but somehow this change breaks clang-tidy/infrastructure/clang-tidy-mac-libcxx.cpp test.

I think we have a patch for that... @ldionne WDYT about upstreaming 9edb9a711503d540cf3126c0fde11ce9a0d9a7aa ?

[arphaman]

I will revert this for now, as this change broke Darwin CI:

https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/37169/

Clang.Tooling.clang-check-mac-libcxx-abspath.cpp
Clang.Tooling.clang-check-mac-libcxx-relpath.cpp

@ldionne has a patch for these tests I believe, please coordinate with him before recommitting this change

[DmitryPolukhin]

@ldionne could you please upstream 9edb9a711503d540cf3126c0fde11ce9a0d9a7aa (I don't know what it is)?
Alternatively I can figure out what to do with this test (it is just copies of the same test with minimal modifications) with a hint how it was resolved to make sure that it worn diverse from want it should be on Darwin.

[ldionne]

In D157283#4614177, @DmitryPolukhin wrote:

@ldionne could you please upstream 9edb9a711503d540cf3126c0fde11ce9a0d9a7aa (I don't know what it is)?
Alternatively I can figure out what to do with this test (it is just copies of the same test with minimal modifications) with a hint how it was resolved to make sure that it worn diverse from want it should be on Darwin.

As explained in https://reviews.llvm.org/D89001#4648241, upstream Clang is the way it should be right now. The intent is that we first look along the clang binary for headers (that way if you install a custom libc++ it will be picked up), and then we fall back to the SDK version. It's just unfortunate that AppleClang has lagged behind a bit and created some confusion, but I believe the latest release should have the exact same behavior as upstream Clang wrt search paths. The commit mentioned above (9edb9a711503d540cf3126c0fde11ce9a0d9a7aa) as actually just making downstream back in sync with upstream. So IMO the correct thing to do is to abandon this change and wait for a new AppleClang that has matching behavior with upstream Clang (it's probably in Xcode 15 RC which came out recently).

[DmitryPolukhin]

In D157283#4648242, @ldionne wrote:

As explained in https://reviews.llvm.org/D89001#4648241, upstream Clang is the way it should be right now. The intent is that we first look along the clang binary for headers (that way if you install a custom libc++ it will be picked up), and then we fall back to the SDK version. It's just unfortunate that AppleClang has lagged behind a bit and created some confusion, but I believe the latest release should have the exact same behavior as upstream Clang wrt search paths. The commit mentioned above (9edb9a711503d540cf3126c0fde11ce9a0d9a7aa) as actually just making downstream back in sync with upstream. So IMO the correct thing to do is to abandon this change and wait for a new AppleClang that has matching behavior with upstream Clang (it's probably in Xcode 15 RC which came out recently).

@ldionne thank you for the reply. Unfortunately current behaviour makes problems for clang-tools like clang-tidy and clangd that read CDBs from compile_commands.json. They start looking headers relative to compiler path specified in CDB but it is better to use path specified in -isysroot (it is what user expects when they specify the option). What is the rationale behind ignoring -isysroot specified in command line? If user would like to use search path relative to compiler, they could just remove -isysroot and get this behaviour.

clang/lib/Driver/ToolChains/Darwin.cpp

void DarwinClang::AddClangCXXStdlibIncludeArgs(
     llvm::opt::ArgStringList &CC1Args) const {
   // The implementation from a base class will pass through the -stdlib to
   // CC1Args.
   // FIXME: this should not be necessary, remove usages in the frontend
   //        (e.g. HeaderSearchOptions::UseLibcxx) and don't pipe -stdlib.
   //        Also check whether this is used for setting library search paths.
   ToolChain::AddClangCXXStdlibIncludeArgs(DriverArgs, CC1Args);
 
-  if (DriverArgs.hasArg(options::OPT_nostdinc, options::OPT_nostdlibinc,
-                        options::OPT_nostdincxx))
+  if (DriverArgs.hasArg(options::OPT_nostdlibinc, options::OPT_nostdincxx))
     return;
 
   llvm::SmallString<128> Sysroot = GetEffectiveSysroot(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: "-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 <sysroot> headers should be preferred over the headers in the toolchain).
 // Ensure that both -isysroot and --sysroot work, and that isysroot has precedence
 // over --sysroot.
 //
 // RUN: %clang -### %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: "-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, -nostdinc++ or -nostdlib will drop both the toolchain
+// Make sure that using -nostdinc++ or -nostdlib will drop both the toolchain
 // C++ include path and the sysroot one.
 //
 // RUN: %clang -### %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 \
 // RUN:     -stdlib=platform \
 // RUN:     -nostdinc \
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
-// RUN:               --check-prefix=CHECK-LIBCXX-NOSTDLIBINC %s
+// RUN:               --check-prefix=CHECK-LIBCXX-NOSTDINC %s
 // CHECK-LIBCXX-NOSTDINC: "-cc1"
-// CHECK-LIBCXX-NOSTDINC-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
+// CHECK-LIBCXX-NOSTDINC: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-NOSTDINC-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 //
 // RUN: %clang -### %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: %clang %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:     -stdlib=libc++ \
 // 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"
 
 // Make sure that on Darwin, we use libc++ header search paths by default even when
 // -stdlib= isn't passed.
 //
 // RUN: %clang -### %s -fsyntax-only 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_usr_cxx_v1 \
 // 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-STDLIB-UNSPECIFIED %s
 // CHECK-LIBCXX-STDLIB-UNSPECIFIED: "-cc1"
 // CHECK-LIBCXX-STDLIB-UNSPECIFIED: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"