[Gnu toolchain] Look at standard GCC paths for libstdcxx by default

[sthibaul]

Summary

Linux' current addLibCxxIncludePaths and addLibStdCxxIncludePaths are
actually almost non-Linux-specific at all, and can be reused almost as such
for all gcc toolchains. Only keep Android/Freescale/Cray hacks in Linux'
version.

[kristina]

Added libcxx maintainers, would like one of them to sign off on this. I presume this is NFCI for Linux so it should be covered by existing regression tests?

clang/lib/Driver/ToolChains/Gnu.cpp

2623

No need for = "" here.

[sthibaul]

Not sure what NFCI means, but for Linux the behavior should just not change at all, and the testsuite indeed behaved exactly the same on Linux indeed.

clang/lib/Driver/ToolChains/Gnu.cpp

2623

(it's the existing code just moved to here actually)

I'm afraid that a not-that-smart compiler would complain that it could be used uninitialized below, I have seen such cases produce warnings before because the compiler was not making the connection with MaxVersion being 0. Are we confident that this will not pose problem?

[kristina]

clang/lib/Driver/ToolChains/Gnu.cpp

2623

std::string has a default constructor, but the int needs to be initialized. I was only referring to the string.

[sthibaul]

clang/lib/Driver/ToolChains/Gnu.cpp

2623

std::string has a default constructor

Ah, ok. Now fixed.

[sylvestre.ledru]

@kristina can we land this patch ? Thanks

[sthibaul]

I have updated the patch to the latest git and re-ran checks. Could somebody have a look at it please?

[sylvestre.ledru]

[Gnu toolchain]
maybe this can be removed in the title?

clang/lib/Driver/ToolChains/Linux.cpp

36

are using a multiarch

[sthibaul]

[Gnu toolchain]
maybe this can be removed in the title?

I thought that commits usually have such kind of title prefixes?

clang/lib/Driver/ToolChains/Linux.cpp

36

Well, I can change that, but it's unrelated to the other change of the patch?

[kristina]

Sorry for responding late, was away. This seems to be tripping up a regression test:

FAIL: Clang :: Driver/cross-linux.c (5312 of 16608)
llvm-project/clang/test/Driver/cross-linux.c:62:26: error: CHECK-MULTI32-X86-64: expected string not found in input
// CHECK-MULTI32-X86-64: "crti.o" "[[gcc_install:.*/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0]]/64{{/|\\\\}}crtbegin.o"

Do you mind looking into it?

[sthibaul]

Which checksuite are you running? I had been using ninja check-all, and I do indeed get unexpected failures, but exactly the same set of failures as when building against master.

[kristina]

I ran check-clang on a x86_64 Ubuntu 18.04 machine (toolchain compiled with compiler-rt as the default CRT and libc++/libc++abi/libunwind_llvm). I think one of the driver tests for multilib stuff may be broken, in which case it should probably be investigated/fixed before this lands. And yes you are right, this test seems to fail with this configuration even without the patch applied. Is this the same test you're referring to (cross-linux.c)?

[sthibaul]

I am actually getting many failures with ninja check-all on my build box:

Expected Passes    : 56258
Expected Failures  : 203
Unsupported Tests  : 1485
Unexpected Failures: 164

I have attached the whole check log:

check.log455 KBDownload

[sthibaul]

(I don't see anything looking like what you have, so can't investigate)

[kristina]

On second look this seems to be caused by compiler-rt being used as the default runtime, which is not accounted for in that particular test.

Test is expecting a GCCesque crtbegin here which is missing with compiler-rt:

"crt1.o" "crti.o" "/o/b/llvm-test/v10.0.4010/bin/../lib/clang/10.0.4010/lib/linux/clang_rt.crtbegin-x86_64.o"

Which fails for obvious reasons:

llvm-project/clang/test/Driver/cross-linux.c:62:26: error: CHECK-MULTI32-X86-64: expected string not found in input
// CHECK-MULTI32-X86-64: "crti.o" "[[gcc_install:.*/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0]]/64{{/|\\\\}}crtbegin.o"

This seems unrelated, apologies for the false alarm, I'll have to look into fixing the test for when compiler-rt is the default CRT myself.

I'm happy to sign off on this. Let me know if you need this committed.

[sthibaul]

Let me know if you need this committed.

Yes, please commit.

[kristina]

For future - Since you seem to put up a lot of (well, almost all) patches related to Hurd support, may I suggest requesting commit access so you can land patches yourself after review?

[MaskRay]

Patch by sthibaul (Samuel Thibault)

@kristina As a belated advice... since LLVM has moved from svn to git, we can ask contributors's names and emails, and then do git commit --amend --author 'name <email@address>'

clang/lib/Driver/ToolChains/Gnu.h

public:
   TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef BoundArch,
                 Action::OffloadKind DeviceOffloadKind) const override;
 
 protected:
   Tool *getTool(Action::ActionClass AC) const override;
   Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
 
+  virtual std::string getMultiarchTriple(const Driver &D,
+                                         const llvm::Triple &TargetTriple,
+                                         StringRef SysRoot) const
+  { return TargetTriple.str(); }
+
   /// \name ToolChain Implementation Helper Functions
   /// @{
 
   /// Check whether the target triple's architecture is 64-bits.
   bool isTarget64Bit() const { return getTriple().isArch64Bit(); }
 
   /// Check whether the target triple's architecture is 32-bits.
   bool isTarget32Bit() const { return getTriple().isArch32Bit(); }
 
   // FIXME: This should be final, but the CrossWindows toolchain does weird
   // things that can't be easily generalized.
   void AddClangCXXStdlibIncludeArgs(
       const llvm::opt::ArgList &DriverArgs,
       llvm::opt::ArgStringList &CC1Args) const override;
 
   virtual void
   addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                         llvm::opt::ArgStringList &CC1Args) const;
   virtual void
   addLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                            llvm::opt::ArgStringList &CC1Args) const;
 
+  bool
+  addGCCLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
+                           llvm::opt::ArgStringList &CC1Args) const;
+
   bool addLibStdCXXIncludePaths(Twine Base, Twine Suffix, StringRef GCCTriple,
                                 StringRef GCCMultiarchTriple,
                                 StringRef TargetMultiarchTriple,
                                 Twine IncludeSuffix,
                                 const llvm::opt::ArgList &DriverArgs,
                                 llvm::opt::ArgStringList &CC1Args) const;
 
   /// @}

clang/lib/Driver/ToolChains/Gnu.cpp

case ToolChain::CST_Libcxx:
     break;
 
   case ToolChain::CST_Libstdcxx:
     addLibStdCxxIncludePaths(DriverArgs, CC1Args);
     break;
   }
 }
 
-void
-Generic_GCC::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
-                                   llvm::opt::ArgStringList &CC1Args) const {
-  // FIXME: The Linux behavior would probaby be a better approach here.
-  addSystemInclude(DriverArgs, CC1Args,
-                   getDriver().SysRoot + "/usr/include/c++/v1");
+static std::string DetectLibcxxIncludePath(llvm::vfs::FileSystem &vfs,
+                                           StringRef base) {
+  std::error_code EC;
+  int MaxVersion = 0;
+  std::string MaxVersionString;
+  for (llvm::vfs::directory_iterator LI = vfs.dir_begin(base, EC), LE;
+       !EC && LI != LE; LI = LI.increment(EC)) {
+    StringRef VersionText = llvm::sys::path::filename(LI->path());
+    int Version;
+    if (VersionText[0] == 'v' &&
+        !VersionText.slice(1, StringRef::npos).getAsInteger(10, Version)) {
+      if (Version > MaxVersion) {
+        MaxVersion = Version;
+        MaxVersionString = VersionText;
+      }
+    }
+  }
+  return MaxVersion ? (base + "/" + MaxVersionString).str() : "";
 }
 
 void
-Generic_GCC::addLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
-                                      llvm::opt::ArgStringList &CC1Args) const {
-  // By default, we don't assume we know where libstdc++ might be installed.
-  // FIXME: If we have a valid GCCInstallation, use it.
+Generic_GCC::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
+  const std::string& SysRoot = getDriver().SysRoot;
+  const std::string LibCXXIncludePathCandidates[] = {
+      DetectLibcxxIncludePath(getVFS(), getDriver().Dir + "/../include/c++"),
+      // If this is a development, non-installed, clang, libcxx will
+      // not be found at ../include/c++ but it likely to be found at
+      // one of the following two locations:
+      DetectLibcxxIncludePath(getVFS(), SysRoot + "/usr/local/include/c++"),
+      DetectLibcxxIncludePath(getVFS(), SysRoot + "/usr/include/c++") };
+  for (const auto &IncludePath : LibCXXIncludePathCandidates) {
+    if (IncludePath.empty() || !getVFS().exists(IncludePath))
+      continue;
+    // Use the first candidate that exists.
+    addSystemInclude(DriverArgs, CC1Args, IncludePath);
+    return;
+  }
 }
 
 /// Helper to add the variant paths of a libstdc++ installation.
 bool Generic_GCC::addLibStdCXXIncludePaths(
     Twine Base, Twine Suffix, StringRef GCCTriple, StringRef GCCMultiarchTriple,
     StringRef TargetMultiarchTriple, Twine IncludeSuffix,
     const ArgList &DriverArgs, ArgStringList &CC1Args) const {
   if (!getVFS().exists(Base + Suffix))
if ((GCCMultiarchTriple.empty() && TargetMultiarchTriple.empty()) ||
     addSystemInclude(DriverArgs, CC1Args,
                      Base + "/" + TargetMultiarchTriple + Suffix);
   }
 
   addSystemInclude(DriverArgs, CC1Args, Base + Suffix + "/backward");
   return true;
 }
 
+bool
+Generic_GCC::addGCCLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
+                                         llvm::opt::ArgStringList &CC1Args) const {
+  // Use GCCInstallation to know where libstdc++ headers are installed.
+  if (!GCCInstallation.isValid())
+    return false;
+
+  // By default, look for the C++ headers in an include directory adjacent to
+  // the lib directory of the GCC installation. Note that this is expect to be
+  // equivalent to '/usr/include/c++/X.Y' in almost all cases.
+  StringRef LibDir = GCCInstallation.getParentLibPath();
+  StringRef InstallDir = GCCInstallation.getInstallPath();
+  StringRef TripleStr = GCCInstallation.getTriple().str();
+  const Multilib &Multilib = GCCInstallation.getMultilib();
+  const std::string GCCMultiarchTriple = getMultiarchTriple(
+      getDriver(), GCCInstallation.getTriple(), getDriver().SysRoot);
+  const std::string TargetMultiarchTriple =
+      getMultiarchTriple(getDriver(), getTriple(), getDriver().SysRoot);
+  const GCCVersion &Version = GCCInstallation.getVersion();
+
+  // The primary search for libstdc++ supports multiarch variants.
+  if (addLibStdCXXIncludePaths(LibDir.str() + "/../include",
+                               "/c++/" + Version.Text, TripleStr,
+                               GCCMultiarchTriple, TargetMultiarchTriple,
+                               Multilib.includeSuffix(), DriverArgs, CC1Args))
+    return true;
+
+  // Otherwise, fall back on a bunch of options which don't use multiarch
+  // layouts for simplicity.
+  const std::string LibStdCXXIncludePathCandidates[] = {
+      // Gentoo is weird and places its headers inside the GCC install,
+      // so if the first attempt to find the headers fails, try these patterns.
+      InstallDir.str() + "/include/g++-v" + Version.Text,
+      InstallDir.str() + "/include/g++-v" + Version.MajorStr + "." +
+          Version.MinorStr,
+      InstallDir.str() + "/include/g++-v" + Version.MajorStr,
+  };
+
+  for (const auto &IncludePath : LibStdCXXIncludePathCandidates) {
+    if (addLibStdCXXIncludePaths(IncludePath, /*Suffix*/ "", TripleStr,
+                                 /*GCCMultiarchTriple*/ "",
+                                 /*TargetMultiarchTriple*/ "",
+                                 Multilib.includeSuffix(), DriverArgs, CC1Args))
+      return true;
+  }
+  return false;
+}
+
+void
+Generic_GCC::addLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
+                                      llvm::opt::ArgStringList &CC1Args) const {
+  addGCCLibStdCxxIncludePaths(DriverArgs, CC1Args);
+}
+
 llvm::opt::DerivedArgList *
 Generic_GCC::TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef,
                            Action::OffloadKind DeviceOffloadKind) const {
 
   // If this tool chain is used for an OpenMP offloading device we have to make
   // sure we always generate a shared library regardless of the commands the
   // user passed to the host. This is required because the runtime library
   // is required to load the device image dynamically at run time.

clang/lib/Driver/ToolChains/Hurd.h

public:
 
   virtual std::string getDynamicLinker(const llvm::opt::ArgList &Args) const;
 
   std::vector<std::string> ExtraOpts;
 
 protected:
   Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
+
+  std::string getMultiarchTriple(const Driver &D,
+                                 const llvm::Triple &TargetTriple,
+                                 StringRef SysRoot) const override;
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H

clang/lib/Driver/ToolChains/Hurd.cpp

 using tools::addPathIfExists;
 
 /// Get our best guess at the multiarch triple for a target.
 ///
 /// Debian-based systems are starting to use a multiarch setup where they use
 /// a target-triple directory in the library and header search paths.
 /// Unfortunately, this triple does not align with the vanilla target triple,
 /// so we provide a rough mapping here.
-static std::string getMultiarchTriple(const Driver &D,
-                                      const llvm::Triple &TargetTriple,
-                                      StringRef SysRoot) {
+std::string Hurd::getMultiarchTriple(const Driver &D,
+                                     StringRef SysRoot) const {
   if (TargetTriple.getArch() == llvm::Triple::x86) {
     // We use the existence of '/lib/<triple>' as a directory to detect some
     // common hurd triples that don't quite match the Clang triple for both
     // 32-bit and 64-bit targets. Multiarch fixes its install triples to these
     // regardless of what the actual target triple is.
     if (D.getVFS().exists(SysRoot + "/lib/i386-gnu"))
       return "i386-gnu";
   }

clang/lib/Driver/ToolChains/Linux.h

public:
   Linux(const Driver &D, const llvm::Triple &Triple,
         const llvm::opt::ArgList &Args);
 
   bool HasNativeLLVMSupport() const override;
 
   void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                             llvm::opt::ArgStringList &CC1Args) const override;
-  void addLibCxxIncludePaths(
-      const llvm::opt::ArgList &DriverArgs,
-      llvm::opt::ArgStringList &CC1Args) const override;
   void addLibStdCxxIncludePaths(
       const llvm::opt::ArgList &DriverArgs,
       llvm::opt::ArgStringList &CC1Args) const override;
   void AddCudaIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                           llvm::opt::ArgStringList &CC1Args) const override;
   void AddIAMCUIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                            llvm::opt::ArgStringList &CC1Args) const override;
   CXXStdlibType GetDefaultCXXStdlibType() const override;
   bool isPIEDefault() const override;
   bool isNoExecStackDefault() const override;
   bool IsMathErrnoDefault() const override;
   SanitizerMask getSupportedSanitizers() const override;
   void addProfileRTLibs(const llvm::opt::ArgList &Args,
                         llvm::opt::ArgStringList &CmdArgs) const override;
   virtual std::string computeSysRoot() const;
 
   virtual std::string getDynamicLinker(const llvm::opt::ArgList &Args) const;
 
   std::vector<std::string> ExtraOpts;
 
 protected:
   Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
+
+  std::string getMultiarchTriple(const Driver &D,
+                                 const llvm::Triple &TargetTriple,
+                                 StringRef SysRoot) const override;
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_LINUX_H

clang/lib/Driver/ToolChains/Linux.cpp

 using namespace clang::driver::toolchains;
 using namespace clang;
 using namespace llvm::opt;
 
 using tools::addPathIfExists;
 
 /// Get our best guess at the multiarch triple for a target.
 ///
 /// Debian-based systems are starting to use a multiarch setup where they use
 /// a target-triple directory in the library and header search paths.
 /// Unfortunately, this triple does not align with the vanilla target triple,
 /// so we provide a rough mapping here.
-static std::string getMultiarchTriple(const Driver &D,
+std::string Linux::getMultiarchTriple(const Driver &D,
                                       const llvm::Triple &TargetTriple,
-                                      StringRef SysRoot) {
+                                      StringRef SysRoot) const {
   llvm::Triple::EnvironmentType TargetEnvironment =
       TargetTriple.getEnvironment();
   bool IsAndroid = TargetTriple.isAndroid();
   bool IsMipsR6 = TargetTriple.getSubArch() == llvm::Triple::MipsSubArch_r6;
   bool IsMipsN32Abi = TargetTriple.getEnvironment() == llvm::Triple::GNUABIN32;
 
   // For most architectures, just use whatever we have rather than trying to be
   // clever.
void Linux::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
   addExternCSystemInclude(DriverArgs, CC1Args, SysRoot + "/include");
 
   addExternCSystemInclude(DriverArgs, CC1Args, SysRoot + "/usr/include");
 
   if (!DriverArgs.hasArg(options::OPT_nobuiltininc) && getTriple().isMusl())
     addSystemInclude(DriverArgs, CC1Args, ResourceDirInclude);
 }
 
-static std::string DetectLibcxxIncludePath(llvm::vfs::FileSystem &vfs,
-                                           StringRef base) {
-  std::error_code EC;
-  int MaxVersion = 0;
-  std::string MaxVersionString = "";
-  for (llvm::vfs::directory_iterator LI = vfs.dir_begin(base, EC), LE;
-       !EC && LI != LE; LI = LI.increment(EC)) {
-    StringRef VersionText = llvm::sys::path::filename(LI->path());
-    int Version;
-    if (VersionText[0] == 'v' &&
-        !VersionText.slice(1, StringRef::npos).getAsInteger(10, Version)) {
-      if (Version > MaxVersion) {
-        MaxVersion = Version;
-        MaxVersionString = VersionText;
-      }
-    }
-  }
-  return MaxVersion ? (base + "/" + MaxVersionString).str() : "";
-}
-
-void Linux::addLibCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
+void Linux::addLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
                                      llvm::opt::ArgStringList &CC1Args) const {
-  const std::string& SysRoot = computeSysRoot();
-  const std::string LibCXXIncludePathCandidates[] = {
-      DetectLibcxxIncludePath(getVFS(), getDriver().Dir + "/../include/c++"),
-      // If this is a development, non-installed, clang, libcxx will
-      // not be found at ../include/c++ but it likely to be found at
-      // one of the following two locations:
-      DetectLibcxxIncludePath(getVFS(), SysRoot + "/usr/local/include/c++"),
-      DetectLibcxxIncludePath(getVFS(), SysRoot + "/usr/include/c++") };
-  for (const auto &IncludePath : LibCXXIncludePathCandidates) {
-    if (IncludePath.empty() || !getVFS().exists(IncludePath))
-      continue;
-    // Use the first candidate that exists.
-    addSystemInclude(DriverArgs, CC1Args, IncludePath);
+  // Try generic GCC detection first.
+  if (Generic_GCC::addGCCLibStdCxxIncludePaths(DriverArgs, CC1Args))
     return;
-  }
-}
 
-void Linux::addLibStdCxxIncludePaths(const llvm::opt::ArgList &DriverArgs,
-                                     llvm::opt::ArgStringList &CC1Args) const {
   // We need a detected GCC installation on Linux to provide libstdc++'s
-  // headers.
+  // headers in odd Linuxish places.
   if (!GCCInstallation.isValid())
     return;
 
-  // By default, look for the C++ headers in an include directory adjacent to
-  // the lib directory of the GCC installation. Note that this is expect to be
-  // equivalent to '/usr/include/c++/X.Y' in almost all cases.
   StringRef LibDir = GCCInstallation.getParentLibPath();
-  StringRef InstallDir = GCCInstallation.getInstallPath();
   StringRef TripleStr = GCCInstallation.getTriple().str();
   const Multilib &Multilib = GCCInstallation.getMultilib();
-  const std::string GCCMultiarchTriple = getMultiarchTriple(
-      getDriver(), GCCInstallation.getTriple(), getDriver().SysRoot);
-  const std::string TargetMultiarchTriple =
-      getMultiarchTriple(getDriver(), getTriple(), getDriver().SysRoot);
   const GCCVersion &Version = GCCInstallation.getVersion();
 
-  // The primary search for libstdc++ supports multiarch variants.
-  if (addLibStdCXXIncludePaths(LibDir.str() + "/../include",
-                               "/c++/" + Version.Text, TripleStr,
-                               GCCMultiarchTriple, TargetMultiarchTriple,
-                               Multilib.includeSuffix(), DriverArgs, CC1Args))
-    return;
-
-  // Otherwise, fall back on a bunch of options which don't use multiarch
-  // layouts for simplicity.
   const std::string LibStdCXXIncludePathCandidates[] = {
-      // Gentoo is weird and places its headers inside the GCC install,
-      // so if the first attempt to find the headers fails, try these patterns.
-      InstallDir.str() + "/include/g++-v" + Version.Text,
-      InstallDir.str() + "/include/g++-v" + Version.MajorStr + "." +
-          Version.MinorStr,
-      InstallDir.str() + "/include/g++-v" + Version.MajorStr,
       // Android standalone toolchain has C++ headers in yet another place.
       LibDir.str() + "/../" + TripleStr.str() + "/include/c++/" + Version.Text,
       // Freescale SDK C++ headers are directly in <sysroot>/usr/include/c++,
       // without a subdirectory corresponding to the gcc version.
       LibDir.str() + "/../include/c++",
       // Cray's gcc installation puts headers under "g++" without a
       // version suffix.
       LibDir.str() + "/../include/g++",