Add libc++ header path for DarwinClang builds

[hintonda]

Summary

Current behavior doesn't add c++ header path if libc++ wasn't installed via macports in /usr. This change will try to locate c++ headers from various locations, including in tree, via macports, sysroot, and default Xcode location -- which is how my configuration finds it.

This probably needs a bit of work, especially wrt other configurations, so feedback would be appreciated..

+Bob Wilson who added the -stdlib=libc++ logic

[hintonda]

Removed redundant code from InitHeaderSearch.cpp, it's handled more cleanly in AddClangCXXStdlibIncludeArgs.

Also, newer versions of OSX add libc++ as the default. Perhaps this could be done in cmake, as well as passing the default path to the c++ include directory used by the version of clang used to compile clang. This directory can be found by running xcrun -f clang and munging the path, e.g.,

LIBCXX_INCLUDE_DIR=$(dirname $(dirname $(xcrun -f clang))..)/include/c++/v1

[hintonda]

Not sure why arc removed some reviewers when I added the latest diff, but
I've added them back.

Sorry for the noise...

[kubamracek]

In what scenario exactly are you seeing an issue? If it's a just-built clang that can't find C++ headers, then you should just build the libcxx project alongside.

lib/Driver/ToolChains.cpp

290–295

This is just a default location of Xcode.app and it's wrong to hardcode it, because the user can install Xcode to different locations (or have multiple installations). Even letting CMake find the path will be wrong, because the location of Xcode is a user's choice.

[hintonda]

In D12646#241308, @kubabrecka wrote:

In what scenario exactly are you seeing an issue? If it's a just-built clang that can't find C++ headers, then you should just build the libcxx project alongside.

clang++ defaults to -stdlib=libc++ for newer versions of MacOS (see ToolChains.cpp:902), but since there isn't a way to pass corresponding include path via cmake, clang++ doesn't work out of the box, i.e., it only checks /usr/include/c++/v1, not the path relative to the version of clang used to build it.

I first noticed it when I tried to run the version of clang-tidy I'd just built and found I had to pass the path for it to find iostream.

This is just a default location of Xcode.app and it's wrong to hardcode it, because the user can install Xcode to different locations (or have multiple installations). Even letting CMake find the path will be wrong, because the location of Xcode is a user's choice.

Yes, but if you allow the user to pass the include path via cmake, how could that be wrong? This is equivalent to passing GCC_INSTALL_PREFIX so clang can find the gcc headers for libstdc++.

The user can always override it if they want to use a different version, but right now, overriding the location is the only way to make it work. If we're not going to generate a working version using libc++, perhaps we shouldn't override -stdlib=libc++ unless the appropriate headers can't be found.

[thakis]

In D12646#241318, @hintonda wrote:

In D12646#241308, @kubabrecka wrote:

In what scenario exactly are you seeing an issue? If it's a just-built clang that can't find C++ headers, then you should just build the libcxx project alongside.

clang++ defaults to -stdlib=libc++ for newer versions of MacOS (see ToolChains.cpp:902), but since there isn't a way to pass corresponding include path via cmake, clang++ doesn't work out of the box, i.e., it only checks /usr/include/c++/v1, not the path relative to the version of clang used to build it.

I first noticed it when I tried to run the version of clang-tidy I'd just built and found I had to pass the path for it to find iostream.

I think it works if you a) check out libcxx into llvm/projects/libcxx b) run make install and c) run the binary from your install directory.

[kubamracek]

I first noticed it when I tried to run the version of clang-tidy I'd just built and found I had to pass the path for it to find iostream.

I think it works if you a) check out libcxx into llvm/projects/libcxx b) run make install and c) run the binary from your install directory.

I don't think b) and c) are necessary. At least for CMake builds, if you checkout libcxx, the output directory will get the C++ headers in the right place.

[hintonda]

I think it works if you a) check out libcxx into llvm/projects/libcxx b) run make install and c) run the binary from your install directory.

Yes, that works. But why shouldn't users be able to use the installed version?

Also, if you want to force users to either build libc++ or have the headers installed in /usr/include/c++/v1, then cmake should fail if neither is the case.

[bob.wilson]

On Darwin platforms, the libc++ headers are expected to be installed alongside clang. If you're not doing that, then you're building it wrong. Adding more fallback options for finding the headers just makes things worse, because instead of a clear failure, you're more likely to get something that works but behaves badly in a subtle way (e.g., due to using really old libc++ headers).

I don't think this is the right thing to do.

[hintonda]

In D12646#241493, @bob.wilson wrote:

On Darwin platforms, the libc++ headers are expected to be installed alongside clang. If you're not doing that, then you're building it wrong. Adding more fallback options for finding the headers just makes things worse, because instead of a clear failure, you're more likely to get something that works but behaves badly in a subtle way (e.g., due to using really old libc++ headers).

I don't think this is the right thing to do.

Okay, I'm comfortable with this solution. I'll work up a diff that makes cmake fail if libc++ isn't included in the build.

lib/Driver/ToolChains.h

public:
 
   void AddCCKextLibArgs(const llvm::opt::ArgList &Args,
                         llvm::opt::ArgStringList &CmdArgs) const override;
 
   void addClangWarningOptions(llvm::opt::ArgStringList &CC1Args) const override;
 
   void AddLinkARCArgs(const llvm::opt::ArgList &Args,
                       llvm::opt::ArgStringList &CmdArgs) const override;
+
+  void AddClangCXXStdlibIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+                                    llvm::opt::ArgStringList &CC1Args) const override;
   /// }
 
 private:
   void AddLinkSanitizerLibArgs(const llvm::opt::ArgList &Args,
                                llvm::opt::ArgStringList &CmdArgs,
                                StringRef Sanitizer) const;
 };
 

lib/Driver/ToolChains.cpp

else if (isTargetIPhoneOS())
     P += "iphoneos";
   else
     P += "macosx";
   P += ".a";
 
   CmdArgs.push_back(Args.MakeArgString(P));
 }
 
+void DarwinClang::AddClangCXXStdlibIncludeArgs(
+    const llvm::opt::ArgList &DriverArgs,
+    llvm::opt::ArgStringList &CC1Args) const {
+  if (DriverArgs.hasArg(options::OPT_nostdlibinc) ||
+      DriverArgs.hasArg(options::OPT_nostdincxx))
+    return;
+
+  DriverArgs.AddAllArgs(CC1Args, options::OPT_stdlib_EQ);
+
+  // Check if libc++ has been enabled and provide its include paths if so.
+  if (GetCXXStdlibType(DriverArgs) == ToolChain::CST_Libcxx) {
+    const std::string LibCXXIncludePathCandidates[] = {
+      // The primary location is within the Clang installation.
+      // FIXME: We shouldn't hard code 'v1' here to make Clang future proof to
+      // newer ABI versions.
+      getDriver().Dir + "/../include/c++/v1",
+
+      // Try sysroot, e.g., DEFAULT_SYSROOT, if set
+      getDriver().SysRoot + "/usr/include/c++/v1",
+
+      // libc++ was installed via macports
+      "/usr/include/c++/v1",
+
+      // FIXME: We should really let cmake find this path, e.g.,
+      // assuming we were compiled with clang:
+      // LIBCXX_INCLUDE_DIR=$(dirname $(dirname $(xcrun -f clang))..)/include/c++/v1
+      // libc++ installed along side clang
+      "/Applications/Xcode.app/Contents/Developer/Toolchains/"
+      "XcodeDefault.xctoolchain/usr/include/c++/v1"
+    };
+    for (const auto &IncludePath : LibCXXIncludePathCandidates) {
+      if (!llvm::sys::fs::exists(IncludePath))
+        continue;
+      // Add the first candidate that exists.
+      addSystemInclude(DriverArgs, CC1Args, IncludePath);
+      break;
+    }
+  } else {
+    // FIXME: The libstdc++ include paths are currently added in
+    // InitHeaderSearch::AddDefaultCPlusPlusIncludePaths().
+    // Should they be moved here?
+  }
+}
+
 void MachO::AddLinkRuntimeLib(const ArgList &Args, ArgStringList &CmdArgs,
                               StringRef DarwinLibName, bool AlwaysLink,
                               bool IsEmbedded, bool AddRPath) const {
   SmallString<128> Dir(getDriver().ResourceDir);
   llvm::sys::path::append(Dir, "lib", IsEmbedded ? "macho_embedded" : "darwin");
 
   SmallString<128> P(Dir);
   llvm::sys::path::append(P, DarwinLibName);

lib/Frontend/InitHeaderSearch.cpp

if (triple.getEnvironment() != llvm::Triple::Cygnus ||
         triple.isOSBinFormatMachO())
       return;
     break;
   }
 
   if (Lang.CPlusPlus && 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);
-        }
-      }
       // On Solaris, include the support directory for things like xlocale and
       // fudged system headers.
       if (triple.getOS() == llvm::Triple::Solaris) 
         AddPath("/usr/include/c++/v1/support/solaris", CXXSystem, false);
       
       AddPath("/usr/include/c++/v1", CXXSystem, false);
     } else {
       AddDefaultCPlusPlusIncludePaths(triple, HSOpts);