Move detection of libc++ include dirs to Driver on MacOS

[ilya-biryukov]

Summary

The intention is to make the tools replaying compilations from 'compile_commands.json'
(clang-tidy, clangd, etc.) find the same standard library as the original compiler
specified in 'compile_commands.json'.

Previously, the library detection logic was in the frontend (InitHeaderSearch.cpp) and relied
on the value of resource dir as an approximation of the compiler install dir. The new logic
uses the actual compiler install dir and is performed in the driver. This is consistent with
the C++ standard library detection on other platforms and allows to override the resource dir
in the tools using the compile_commands.json without altering the
standard library detection mechanism. The tools have to override the resource dir to make sure
they use a consistent version of the builtin headers.

There is still logic in InitHeaderSearch that attemps to add the absolute includes for the
the C++ standard library, so we keep passing the -stdlib=libc++ from the driver to the frontend
via cc1 args to avoid breaking that. In the long run, we should move this logic to the driver too,
but it could potentially break the library detection on other systems, so we don't tackle it in this
patch to keep its scope manageable.

This is a second attempt to fix the issue, first one was commited in r346652 and reverted in r346675.
The original fix relied on an ad-hoc propagation (bypassing the cc1 flags) of the install dir from the
driver to the frontend's HeaderSearchOptions. Unsurpisingly, the propagation was incomplete, it broke
the libc++ detection in clang itself, which caused LLDB tests to break.

The LLDB tests pass with new fix.

[arphaman]

I don't think we want to move the logic to add a libc++ path to the driver. -cc1 with -resource-dir and -stdlib=libc++ should still work as before. In this case the previous patch is better, except it should not set InstalledDir except when needed (e.g. for tooling when working with a CDB that has an absolute path to the compiler), so when InstalledDir is empty it should fallback to the current logic in InitHeaderSearch.cpp. That should solve the issues we had.

[ldionne]

In D54630#1301283, @arphaman wrote:

I don't think we want to move the logic to add a libc++ path to the driver.

My opinion is not very educated because I don't have a lot of experience/knowledge of Clang, but from my perspective it does make sense to limit how much knowledge of the standard library path the front end needs to have. IOW, having the driver figure it out and then pass include paths to the front-end just like for any other library does make sense from my limited point of view. Again -- this is not a practical argument, it's just from a conceptual perspective.

[ilya-biryukov]

In D54630#1301283, @arphaman wrote:

I don't think we want to move the logic to add a libc++ path to the driver. -cc1 with -resource-dir and -stdlib=libc++ should still work as before. In this case the previous patch is better, except it should not set InstalledDir except when needed (e.g. for tooling when working with a CDB that has an absolute path to the compiler), so when InstalledDir is empty it should fallback to the current logic in InitHeaderSearch.cpp. That should solve the issues we had.

Why not?
@ldionne made a good point: there is value in keeping the frontend separate from the logic to detect system libraries and other toolchain-specific things.

This is how other toolchains do it (Linux, etc.). It's more straightforward to the user (one can inspect the -cc1 args produced by the driver), this is where the logic to detect linker dependencies is. It seems that was also the plan of some developers looking in that area: the test I've edited suggests this should be moved to the driver (darwin-stdlib.cpp, it says: If and when we change to finding them in the driver ...), there's a bug corresponding bug to do this: http://llvm.org/PR30548.
I might not have full information, but my understanding is that -resource-dir has a pretty well-defined semantics: it should be used to find the compiler resources, i.e. only the buit-in headers and not the C++ standard library.

What are the reasons we need to keep the old behavior?

[EricWF]

My two cents:

I needed something like this when trying to automatically detect if libc++ supported things like sized deallocation, so I would like to see this change go through.
(My understanding was that libc++ added a __libcpp_version file for just this reason, but it's hard to detect because the include path computation is done after we initialize the language options)

[arphaman]

In D54630#1302566, @ilya-biryukov wrote:

In D54630#1301283, @arphaman wrote:

I don't think we want to move the logic to add a libc++ path to the driver. -cc1 with -resource-dir and -stdlib=libc++ should still work as before. In this case the previous patch is better, except it should not set InstalledDir except when needed (e.g. for tooling when working with a CDB that has an absolute path to the compiler), so when InstalledDir is empty it should fallback to the current logic in InitHeaderSearch.cpp. That should solve the issues we had.

Why not?
@ldionne made a good point: there is value in keeping the frontend separate from the logic to detect system libraries and other toolchain-specific things.

This is how other toolchains do it (Linux, etc.). It's more straightforward to the user (one can inspect the -cc1 args produced by the driver), this is where the logic to detect linker dependencies is. It seems that was also the plan of some developers looking in that area: the test I've edited suggests this should be moved to the driver (darwin-stdlib.cpp, it says: If and when we change to finding them in the driver ...), there's a bug corresponding bug to do this: http://llvm.org/PR30548.
I might not have full information, but my understanding is that -resource-dir has a pretty well-defined semantics: it should be used to find the compiler resources, i.e. only the buit-in headers and not the C++ standard library.

What are the reasons we need to keep the old behavior?

Sounds convincing.
@dexonsmith What do you think?

@ilya-biryukov I'm going to do some internal testing to see if we uncover any issues.

[dexonsmith]

In D54630#1308605, @arphaman wrote:

Sounds convincing.
@dexonsmith What do you think?

Besides maintaining correct behaviour, I think the most important thing here is that the code organization is logical. Header search is complicated and we should be trying to make/keep it simple. I'm a little skeptical that splitting this logic between cc1 and the driver will simplify things, but I haven't looked in detail and I'll defer to your (collective) judgement.

@ilya-biryukov I'm going to do some internal testing to see if we uncover any issues.

I've added you as a blocking reviewer since I think we should work through any uncovered issues pre-commit.

[ilya-biryukov]

In D54630#1308671, @dexonsmith wrote:

I'm a little skeptical that splitting this logic between cc1 and the driver will simplify things, but I haven't looked in detail and I'll defer to your (collective) judgement.

Most of the toolchain search logic is in the driver, so moving from the frontend to the driver is actually a step in moving the logic into one place.

In D54630#1308605, @arphaman wrote:

@ilya-biryukov I'm going to do some internal testing to see if we uncover any issues.

Many thanks, will be waiting for the results of the internal testing.

[ilya-biryukov]

@arphaman, did you have a chance to run the tests? There's no rush, just wanted to know whether we have any data if this change breaks anything in practice?

[arphaman]

So far everything looks fine, but I have to rerun a couple of things due to infrastructural issues. I should have the final results next Monday.

[ilya-biryukov]

In D54630#1315168, @arphaman wrote:

So far everything looks fine, but I have to rerun a couple of things due to infrastructural issues. I should have the final results next Monday.

Thanks for the update. Will be waiting for the final results.

[arphaman]

LGTM

[arphaman]

Please mention this change in the release notes though.

[ilya-biryukov]

D55322 is a review for release notes

[ormris]

Hi Ilya,

This commit is causing a test failure on the PS4 bot. Could you take a look?

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/42251

Thanks,
Matthew

[ilya-biryukov]

Will take a look.
For reference: the particular failing test was added in r351222 (D56680), not in this change.

[ilya-biryukov]

r351334 should fix this.
However, this is an indication that libc++ living alongside the compiler (i.e. in /bin/../include/c++/v1) cannot be found on PS4.
I am not familiar with PS4 myself, but I believe it would be reasonable to assume finding libc++ living alongside the clang itself should work on all platforms, including PS4.

lib/Driver/ToolChains/Darwin.h

public:
   /// @name Apple ToolChain Implementation
   /// {
 
   RuntimeLibType GetRuntimeLibType(const llvm::opt::ArgList &Args) const override;
 
   void AddLinkRuntimeLibArgs(const llvm::opt::ArgList &Args,
                              llvm::opt::ArgStringList &CmdArgs) const override;
 
+  void AddClangCXXStdlibIncludeArgs(
+      const llvm::opt::ArgList &DriverArgs,
+      llvm::opt::ArgStringList &CC1Args) const override;
+
   void AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
                            llvm::opt::ArgStringList &CmdArgs) const override;
 
   void AddCCKextLibArgs(const llvm::opt::ArgList &Args,
                         llvm::opt::ArgStringList &CmdArgs) const override;
 
   void addClangWarningOptions(llvm::opt::ArgStringList &CC1Args) const override;
 

lib/Driver/ToolChains/Darwin.cpp

if (SDK.size() > 0) {
       StringRef SDKName = SDK.slice(0, StartVer);
       if (!SDKName.startswith(getPlatformFamily()))
         getDriver().Diag(diag::warn_incompatible_sysroot)
             << SDKName << getPlatformFamily();
     }
   }
 }
 
+void DarwinClang::AddClangCXXStdlibIncludeArgs(
+    const llvm::opt::ArgList &DriverArgs,
+    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.
+  ToolChain::AddClangCXXStdlibIncludeArgs(DriverArgs, CC1Args);
+
+  if (DriverArgs.hasArg(options::OPT_nostdlibinc) ||
+      DriverArgs.hasArg(options::OPT_nostdincxx))
+    return;
+
+  switch (GetCXXStdlibType(DriverArgs)) {
+  case ToolChain::CST_Libcxx: {
+    llvm::StringRef InstallDir = getDriver().getInstalledDir();
+    if (InstallDir.empty())
+      break;
+    // On Darwin, libc++ may be installed alongside the compiler in
+    // include/c++/v1.
+    // Get from 'foo/bin' to 'foo'.
+    SmallString<128> P = llvm::sys::path::parent_path(InstallDir);
+    // Get to 'foo/include/c++/v1'.
+    llvm::sys::path::append(P, "include", "c++", "v1");
+    addSystemInclude(DriverArgs, CC1Args, P);
+    break;
+  }
+  case ToolChain::CST_Libstdcxx:
+    // FIXME: should we do something about it?
+    break;
+  }
+}
 void DarwinClang::AddCXXStdlibLibArgs(const ArgList &Args,
                                       ArgStringList &CmdArgs) const {
   CXXStdlibType Type = GetCXXStdlibType(Args);
 
   switch (Type) {
   case ToolChain::CST_Libcxx:
     CmdArgs.push_back("-lc++");
     break;

lib/Frontend/InitHeaderSearch.cpp

if (triple.getEnvironment() != llvm::Triple::Cygnus ||
         triple.isOSBinFormatMachO())
       return;
     break;
   }
 
   if (Lang.CPlusPlus && !Lang.AsmPreprocessor &&
       HSOpts.UseStandardCXXIncludes && HSOpts.UseStandardSystemIncludes) {
     if (HSOpts.UseLibcxx) {
-      if (triple.isOSDarwin()) {
-        // On Darwin, libc++ may be installed alongside the compiler in
-        // include/c++/v1.
-        if (!HSOpts.ResourceDir.empty()) {
-          // Remove version from foo/lib/clang/version
-          StringRef NoVer = llvm::sys::path::parent_path(HSOpts.ResourceDir);
-          // Remove clang from foo/lib/clang
-          StringRef Lib = llvm::sys::path::parent_path(NoVer);
-          // Remove lib from foo/lib
-          SmallString<128> P = llvm::sys::path::parent_path(Lib);
-
-          // Get foo/include/c++/v1
-          llvm::sys::path::append(P, "include", "c++", "v1");
-          AddUnmappedPath(P, CXXSystem, false);
-        }
-      }
       AddPath("/usr/include/c++/v1", CXXSystem, false);
     } else {
       AddDefaultCPlusPlusIncludePaths(Lang, triple, HSOpts);
     }
   }
 
   AddDefaultCIncludePaths(triple, HSOpts);
 

test/Driver/darwin-stdlib.cpp

 // This test will fail if CLANG_DEFAULT_CXX_STDLIB is set to anything different
 // than the platform default. (see https://llvm.org/bugs/show_bug.cgi?id=30548)
 // XFAIL: default-cxx-stdlib-set
 
-// RUN: %clang -target x86_64-apple-darwin -arch arm64 -miphoneos-version-min=7.0 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBCXX
-// RUN: %clang -target x86_64-apple-darwin -mmacosx-version-min=10.8 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBSTDCXX
-// RUN: %clang -target x86_64-apple-darwin -mmacosx-version-min=10.9 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBCXX
-// RUN: %clang -target x86_64-apple-darwin -arch armv7s -miphoneos-version-min=6.1 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBSTDCXX
-// RUN: %clang -target x86_64-apple-darwin -arch armv7s -miphoneos-version-min=7.0 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBCXX
-// RUN: %clang -target x86_64-apple-darwin -arch armv7k %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBCXX
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %S/Inputs/darwin_toolchain_tree/bin/ -arch arm64 -miphoneos-version-min=7.0 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBCXX
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %S/Inputs/darwin_toolchain_tree/bin/ -mmacosx-version-min=10.8 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBSTDCXX
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %S/Inputs/darwin_toolchain_tree/bin/ -mmacosx-version-min=10.9 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBCXX
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %S/Inputs/darwin_toolchain_tree/bin/ -arch armv7s -miphoneos-version-min=6.1 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBSTDCXX
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %S/Inputs/darwin_toolchain_tree/bin/ -arch armv7s -miphoneos-version-min=7.0 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBCXX
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %S/Inputs/darwin_toolchain_tree/bin/ -arch armv7k %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-LIBCXX
 
 // The purpose of this test is that the libc++ headers should be found
-// properly. At the moment this is done by passing -stdlib=libc++ down to the
-// cc1 invocation. If and when we change to finding them in the driver this test
-// should reflect that.
+// properly. We also pass -stdlib=libc++ to make sure the logic to add the
+// optional absolute include for libc++ from InitHeaderSearch.cpp also fires.
 
-// CHECK-LIBCXX: -stdlib=libc++
+// CHECK-LIBCXX: "-stdlib=libc++"
+// CHECK-LIBCXX: "-internal-isystem" "{{[^"]*}}{{[/\\]}}Inputs{{[/\\]}}darwin_toolchain_tree{{[/\\]}}bin{{[/\\]}}include{{[/\\]}}c++{{[/\\]}}v1"
 
 // CHECK-LIBSTDCXX-NOT: -stdlib=libc++
 // CHECK-LIBSTDCXX-NOT: -stdlib=libstdc++
+// CHECK-LIBSTDCXX-NOT: -internal-isystem

test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector

+class vector {};

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.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+//
+// Install the mock libc++ (simulates the libc++ directory structure).
+// RUN: cp -r %S/Inputs/mock-libcxx %t/
+//
+// 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
+// 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;

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.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+//
+// Install the mock libc++ (simulates the libc++ directory structure).
+// RUN: cp -r %S/Inputs/mock-libcxx %t/
+//
+// 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
+// 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;