[Gnu toolchain] Move GCC multilib/multiarch paths support from Linux to Gnu

[sthibaul]

Summary

The current code for GNU/Linux is actually completely generic, and can be moved to Gnu, so it can benefit GNU/Hurd and GNU/kFreeBSD

[sthibaul]

There is no behavior change, I checked for no regression on GNU/Linux with ninja check-all.

[lebedev.ri]

1. Please clang-format your changes
2. Hurd stuff is not a NFC refactoring, but adding some new feature - tests missing, i think?

[sthibaul]

Right, here is an updated patch

[kristina]

I feel like in this specific case it may be worth splitting this into two patches:

• Factoring certain stuff out of Linux.cpp into Gnu.(cpp|h).
• Adding that machinery to Hurd.cpp with related tests.

Also forgive me if I'm wrong but isn't Hurd only limited to one GCC target triple (i386-gnu) for the time being? A lot of the factored out bits seem to be still only applicable to Linux at a glance, in which case I'm not entirely sure of the rationale behind most of the refactoring. Since most people (including me) are unlikely to be intimately familiar with the directory/library layout on Hurd, is it possible to provide some more background on the Hurd-specific side of the patch?

clang/lib/Driver/ToolChains/Gnu.h

324

Maybe rename AddGCCClangSystemIncludeArgs to AddMultilibIncludeArgs for brevity/clarity?

[sthibaul]

I feel like in this specific case it may be worth splitting this into two patches:

Alright, doing so.

isn't Hurd only limited to one GCC target triple (i386-gnu) for the time being?

Right now, yes, but we'll want to bootstrap a 64bit port sooner or later. Better have that part ready in llvm to lessen the work of the bootstrap.

A lot of the factored out bits seem to be still only applicable to Linux at a glance,

? Everything I have seen in here looked to me only gcc-ish way of handling multilib and multiarch. It is used the same on other GNU systems using gcc: not only GNU/Hurd, but also GNU/kFreeBSD. I do not see anything specific to Linux there, only gcc-specific, which is the point of Gnu.cpp :)

most people (including me) are unlikely to be intimately familiar with the directory/library layout on Hurd, is it possible to provide some more background on the Hurd-specific side of the patch?

It is just a matter of applying the same gcc-ish file layout which is currently missing from the existing Hurd.cpp, thus making multilib/multiarch fail to work. I don't really know what to explain beyond that: it's just the GNU toolchain way, there is no difference between GNU/Linux and GNU/Hurd in that regard. Do you perhaps have more precise questions?

[sthibaul]

I feel like in this specific case it may be worth splitting this into two patches:

Alright, doing so.

It is now on https://reviews.llvm.org/D74282

I however didn't find how to specify in phabricator a dependency between the two diffs, is that supported in phabricator?

[sthibaul]

I however didn't find how to specify in phabricator a dependency between the two diffs, is that supported in phabricator?

Ah, it can be set as child revision after creating the diff, done so.

[sthibaul]

Hello, any news on this?

[MaskRay]

Moving functions from Linux.cpp to Gnu.cpp is definitely appropriate. Have you made any adjustment?

[phosek]

clang/lib/Driver/ToolChains/Gnu.cpp

2697

The class already has GCC in the name, so there's no need to repeat GCC in the name of the method, i.e. this can be just Generic_GCC::AddMultilibPaths. The same for other new methods.

[sthibaul]

In D73845#1971282, @MaskRay wrote:

Moving functions from Linux.cpp to Gnu.cpp is definitely appropriate. Have you made any adjustment?

No, it's really pure code move (and the requested codestyle fixes)

clang/lib/Driver/ToolChains/Gnu.cpp

2697

Alright, fixing so

[MaskRay]

Looks great! You may need to wait for @phosek to confirm.

[sthibaul]

ping @phosek ?

[phosek]

LGTM

[sthibaul]

Thanks! I don't have commit access, could somebody commit this for me?

[MaskRay]

In D73845#2000455, @sthibaul wrote:

Thanks! I don't have commit access, could somebody commit this for me?

Can you provide your name <email@address>? I need this information to run git c --amend --author=

[sthibaul]

@MaskRay : Samuel Thibault <[e-mail]>

clang/lib/Driver/ToolChains/Gnu.h

protected:
   /// @{
 
   /// 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(); }
 
+  void PushPPaths(ToolChain::path_list &PPaths);
+  void AddMultilibPaths(const Driver &D, const std::string &SysRoot,
+                        const std::string &OSLibDir,
+                        const std::string &MultiarchTriple,
+                        path_list &Paths);
+  void AddMultiarchPaths(const Driver &D, const std::string &SysRoot,
+                         const std::string &OSLibDir, path_list &Paths);
+  void AddMultilibIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+                              llvm::opt::ArgStringList &CC1Args) const;
+
   // 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,

clang/lib/Driver/ToolChains/Gnu.cpp

 #include <system_error>
 
 using namespace clang::driver;
 using namespace clang::driver::toolchains;
 using namespace clang;
 using namespace llvm::opt;
 
 using tools::addMultilibFlag;
+using tools::addPathIfExists;
 
 void tools::GnuTool::anchor() {}
 
 static bool forwardToGCC(const Option &O) {
   // Don't forward inputs from the original command line.  They are added from
   // InputInfoList.
   return O.getKind() != Option::InputClass &&
          !O.hasFlag(options::DriverOption) && !O.hasFlag(options::LinkerInput);
if (getTriple().isOSFreeBSD() || getTriple().isOSOpenBSD() ||
         getTriple().isOSSolaris())
       return true;
     return false;
   default:
     return false;
   }
 }
 
+static void addMultilibsFilePaths(const Driver &D, const MultilibSet &Multilibs,
+                                  const Multilib &Multilib,
+                                  StringRef InstallPath,
+                                  ToolChain::path_list &Paths) {
+  if (const auto &PathsCallback = Multilibs.filePathsCallback())
+    for (const auto &Path : PathsCallback(Multilib))
+      addPathIfExists(D, InstallPath + Path, Paths);
+}
+
+void Generic_GCC::PushPPaths(ToolChain::path_list &PPaths) {
+  // Cross-compiling binutils and GCC installations (vanilla and openSUSE at
+  // least) put various tools in a triple-prefixed directory off of the parent
+  // of the GCC installation. We use the GCC triple here to ensure that we end
+  // up with tools that support the same amount of cross compiling as the
+  // detected GCC installation. For example, if we find a GCC installation
+  // targeting x86_64, but it is a bi-arch GCC installation, it can also be
+  // used to target i386.
+  if (GCCInstallation.isValid()) {
+    PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
+                           GCCInstallation.getTriple().str() + "/bin")
+                         .str());
+  }
+}
+
+void Generic_GCC::AddMultilibPaths(const Driver &D,
+                                   const std::string &SysRoot,
+                                   const std::string &OSLibDir,
+                                   const std::string &MultiarchTriple,
+                                   path_list &Paths) {
+  // Add the multilib suffixed paths where they are available.
+  if (GCCInstallation.isValid()) {
+    const llvm::Triple &GCCTriple = GCCInstallation.getTriple();
+    const std::string &LibPath =
+        std::string(GCCInstallation.getParentLibPath());
+
+    // Add toolchain / multilib specific file paths.
+    addMultilibsFilePaths(D, Multilibs, SelectedMultilib,
+                          GCCInstallation.getInstallPath(), Paths);
+
+    // Sourcery CodeBench MIPS toolchain holds some libraries under
+    // a biarch-like suffix of the GCC installation.
+    addPathIfExists(
+        D, GCCInstallation.getInstallPath() + SelectedMultilib.gccSuffix(),
+        Paths);
+
+    // GCC cross compiling toolchains will install target libraries which ship
+    // as part of the toolchain under <prefix>/<triple>/<libdir> rather than as
+    // any part of the GCC installation in
+    // <prefix>/<libdir>/gcc/<triple>/<version>. This decision is somewhat
+    // debatable, but is the reality today. We need to search this tree even
+    // when we have a sysroot somewhere else. It is the responsibility of
+    // whomever is doing the cross build targeting a sysroot using a GCC
+    // installation that is *not* within the system root to ensure two things:
+    //
+    //  1) Any DSOs that are linked in from this tree or from the install path
+    //     above must be present on the system root and found via an
+    //     appropriate rpath.
+    //  2) There must not be libraries installed into
+    //     <prefix>/<triple>/<libdir> unless they should be preferred over
+    //     those within the system root.
+    //
+    // Note that this matches the GCC behavior. See the below comment for where
+    // Clang diverges from GCC's behavior.
+    addPathIfExists(D,
+                    LibPath + "/../" + GCCTriple.str() + "/lib/../" + OSLibDir +
+                        SelectedMultilib.osSuffix(),
+                    Paths);
+
+    // If the GCC installation we found is inside of the sysroot, we want to
+    // prefer libraries installed in the parent prefix of the GCC installation.
+    // It is important to *not* use these paths when the GCC installation is
+    // outside of the system root as that can pick up unintended libraries.
+    // This usually happens when there is an external cross compiler on the
+    // host system, and a more minimal sysroot available that is the target of
+    // the cross. Note that GCC does include some of these directories in some
+    // configurations but this seems somewhere between questionable and simply
+    // a bug.
+    if (StringRef(LibPath).startswith(SysRoot)) {
+      addPathIfExists(D, LibPath + "/" + MultiarchTriple, Paths);
+      addPathIfExists(D, LibPath + "/../" + OSLibDir, Paths);
+    }
+  }
+}
+
+void Generic_GCC::AddMultiarchPaths(const Driver &D,
+                                    const std::string &SysRoot,
+                                    const std::string &OSLibDir,
+                                    path_list &Paths) {
+  // Try walking via the GCC triple path in case of biarch or multiarch GCC
+  // installations with strange symlinks.
+  if (GCCInstallation.isValid()) {
+    addPathIfExists(D,
+                    SysRoot + "/usr/lib/" + GCCInstallation.getTriple().str() +
+                        "/../../" + OSLibDir,
+                    Paths);
+
+    // Add the 'other' biarch variant path
+    Multilib BiarchSibling;
+    if (GCCInstallation.getBiarchSibling(BiarchSibling)) {
+      addPathIfExists(
+          D, GCCInstallation.getInstallPath() + BiarchSibling.gccSuffix(),
+                      Paths);
+    }
+
+    // See comments above on the multilib variant for details of why this is
+    // included even from outside the sysroot.
+    const std::string &LibPath =
+        std::string(GCCInstallation.getParentLibPath());
+    const llvm::Triple &GCCTriple = GCCInstallation.getTriple();
+    const Multilib &Multilib = GCCInstallation.getMultilib();
+    addPathIfExists(
+        D, LibPath + "/../" + GCCTriple.str() + "/lib" + Multilib.osSuffix(),
+                    Paths);
+
+    // See comments above on the multilib variant for details of why this is
+    // only included from within the sysroot.
+    if (StringRef(LibPath).startswith(SysRoot))
+      addPathIfExists(D, LibPath, Paths);
+  }
+}
+
+void Generic_GCC::AddMultilibIncludeArgs(const ArgList &DriverArgs,
+                                         ArgStringList &CC1Args) const {
+  // Add include directories specific to the selected multilib set and multilib.
+  if (GCCInstallation.isValid()) {
+    const auto &Callback = Multilibs.includeDirsCallback();
+    if (Callback) {
+      for (const auto &Path : Callback(GCCInstallation.getMultilib()))
+        addExternCSystemIncludeIfExists(
+            DriverArgs, CC1Args, GCCInstallation.getInstallPath() + Path);
+    }
+  }
+}
+
 void Generic_GCC::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
                                                ArgStringList &CC1Args) const {
   if (DriverArgs.hasArg(options::OPT_nostdlibinc) ||
       DriverArgs.hasArg(options::OPT_nostdincxx))
     return;
 
   switch (GetCXXStdlibType(DriverArgs)) {
   case ToolChain::CST_Libcxx:

clang/lib/Driver/ToolChains/Linux.cpp

if (Triple.getArch() == llvm::Triple::x86_64 &&
     return "libx32";
 
   if (Triple.getArch() == llvm::Triple::riscv32)
     return "lib32";
 
   return Triple.isArch32Bit() ? "lib" : "lib64";
 }
 
-static void addMultilibsFilePaths(const Driver &D, const MultilibSet &Multilibs,
-                                  const Multilib &Multilib,
-                                  StringRef InstallPath,
-                                  ToolChain::path_list &Paths) {
-  if (const auto &PathsCallback = Multilibs.filePathsCallback())
-    for (const auto &Path : PathsCallback(Multilib))
-      addPathIfExists(D, InstallPath + Path, Paths);
-}
-
 Linux::Linux(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
     : Generic_ELF(D, Triple, Args) {
   GCCInstallation.init(Triple, Args);
   Multilibs = GCCInstallation.getMultilibs();
   SelectedMultilib = GCCInstallation.getMultilib();
   llvm::Triple::ArchType Arch = Triple.getArch();
   std::string SysRoot = computeSysRoot();
-
-  // Cross-compiling binutils and GCC installations (vanilla and openSUSE at
-  // least) put various tools in a triple-prefixed directory off of the parent
-  // of the GCC installation. We use the GCC triple here to ensure that we end
-  // up with tools that support the same amount of cross compiling as the
-  // detected GCC installation. For example, if we find a GCC installation
-  // targeting x86_64, but it is a bi-arch GCC installation, it can also be
-  // used to target i386.
-  // FIXME: This seems unlikely to be Linux-specific.
   ToolChain::path_list &PPaths = getProgramPaths();
-  if (GCCInstallation.isValid()) {
-    PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
-                           GCCInstallation.getTriple().str() + "/bin")
-                         .str());
-  }
+
+  Generic_GCC::PushPPaths(PPaths);
 
   Distro Distro(D.getVFS(), Triple);
 
   if (Distro.IsAlpineLinux() || Triple.isAndroid()) {
     ExtraOpts.push_back("-z");
     ExtraOpts.push_back("now");
   }
 
#endif
   // This was determined by running GCC in a fake filesystem, creating all
   // possible permutations of these directories, and seeing which ones it added
   // to the link paths.
   path_list &Paths = getFilePaths();
 
   const std::string OSLibDir = std::string(getOSLibDir(Triple, Args));
   const std::string MultiarchTriple = getMultiarchTriple(D, Triple, SysRoot);
 
-  // Add the multilib suffixed paths where they are available.
-  if (GCCInstallation.isValid()) {
-    const llvm::Triple &GCCTriple = GCCInstallation.getTriple();
-    const std::string &LibPath =
-        std::string(GCCInstallation.getParentLibPath());
-
-    // Add toolchain / multilib specific file paths.
-    addMultilibsFilePaths(D, Multilibs, SelectedMultilib,
-                          GCCInstallation.getInstallPath(), Paths);
-
-    // Sourcery CodeBench MIPS toolchain holds some libraries under
-    // a biarch-like suffix of the GCC installation.
-    addPathIfExists(
-        D, GCCInstallation.getInstallPath() + SelectedMultilib.gccSuffix(),
-        Paths);
-
-    // GCC cross compiling toolchains will install target libraries which ship
-    // as part of the toolchain under <prefix>/<triple>/<libdir> rather than as
-    // any part of the GCC installation in
-    // <prefix>/<libdir>/gcc/<triple>/<version>. This decision is somewhat
-    // debatable, but is the reality today. We need to search this tree even
-    // when we have a sysroot somewhere else. It is the responsibility of
-    // whomever is doing the cross build targeting a sysroot using a GCC
-    // installation that is *not* within the system root to ensure two things:
-    //
-    //  1) Any DSOs that are linked in from this tree or from the install path
-    //     above must be present on the system root and found via an
-    //     appropriate rpath.
-    //  2) There must not be libraries installed into
-    //     <prefix>/<triple>/<libdir> unless they should be preferred over
-    //     those within the system root.
-    //
-    // Note that this matches the GCC behavior. See the below comment for where
-    // Clang diverges from GCC's behavior.
-    addPathIfExists(D, LibPath + "/../" + GCCTriple.str() + "/lib/../" +
-                           OSLibDir + SelectedMultilib.osSuffix(),
-                    Paths);
-
-    // If the GCC installation we found is inside of the sysroot, we want to
-    // prefer libraries installed in the parent prefix of the GCC installation.
-    // It is important to *not* use these paths when the GCC installation is
-    // outside of the system root as that can pick up unintended libraries.
-    // This usually happens when there is an external cross compiler on the
-    // host system, and a more minimal sysroot available that is the target of
-    // the cross. Note that GCC does include some of these directories in some
-    // configurations but this seems somewhere between questionable and simply
-    // a bug.
-    if (StringRef(LibPath).startswith(SysRoot)) {
-      addPathIfExists(D, LibPath + "/" + MultiarchTriple, Paths);
-      addPathIfExists(D, LibPath + "/../" + OSLibDir, Paths);
-    }
-  }
+  Generic_GCC::AddMultilibPaths(D, SysRoot, OSLibDir, MultiarchTriple, Paths);
 
   // Similar to the logic for GCC above, if we currently running Clang inside
   // of the requested system root, add its parent library paths to
   // those searched.
   // FIXME: It's not clear whether we should use the driver's installed
   // directory ('Dir' below) or the ResourceDir.
   if (StringRef(D.Dir).startswith(SysRoot)) {
     addPathIfExists(D, D.Dir + "/../lib/" + MultiarchTriple, Paths);
#endif
   else
     addPathIfExists(D, SysRoot + "/usr/lib/../" + OSLibDir, Paths);
   if (IsRISCV) {
     StringRef ABIName = tools::riscv::getRISCVABI(Args, Triple);
     addPathIfExists(D, SysRoot + "/" + OSLibDir + "/" + ABIName, Paths);
     addPathIfExists(D, SysRoot + "/usr/" + OSLibDir + "/" + ABIName, Paths);
   }
 
-  // Try walking via the GCC triple path in case of biarch or multiarch GCC
-  // installations with strange symlinks.
-  if (GCCInstallation.isValid()) {
-    addPathIfExists(D,
-                    SysRoot + "/usr/lib/" + GCCInstallation.getTriple().str() +
-                        "/../../" + OSLibDir,
-                    Paths);
-
-    // Add the 'other' biarch variant path
-    Multilib BiarchSibling;
-    if (GCCInstallation.getBiarchSibling(BiarchSibling)) {
-      addPathIfExists(D, GCCInstallation.getInstallPath() +
-                             BiarchSibling.gccSuffix(),
-                      Paths);
-    }
-
-    // See comments above on the multilib variant for details of why this is
-    // included even from outside the sysroot.
-    const std::string &LibPath =
-        std::string(GCCInstallation.getParentLibPath());
-    const llvm::Triple &GCCTriple = GCCInstallation.getTriple();
-    const Multilib &Multilib = GCCInstallation.getMultilib();
-    addPathIfExists(D, LibPath + "/../" + GCCTriple.str() + "/lib" +
-                           Multilib.osSuffix(),
-                    Paths);
-
-    // See comments above on the multilib variant for details of why this is
-    // only included from within the sysroot.
-    if (StringRef(LibPath).startswith(SysRoot))
-      addPathIfExists(D, LibPath, Paths);
-  }
+  Generic_GCC::AddMultiarchPaths(D, SysRoot, OSLibDir, Paths);
 
   // Similar to the logic for GCC above, if we are currently running Clang
   // inside of the requested system root, add its parent library path to those
   // searched.
   // FIXME: It's not clear whether we should use the driver's installed
   // directory ('Dir' below) or the ResourceDir.
   if (StringRef(D.Dir).startswith(SysRoot))
     addPathIfExists(D, D.Dir + "/../lib", Paths);
for (StringRef dir : dirs) {
       addExternCSystemInclude(DriverArgs, CC1Args, Prefix + dir);
     }
     return;
   }
 
   // Lacking those, try to detect the correct set of system includes for the
   // target triple.
 
-  // Add include directories specific to the selected multilib set and multilib.
-  if (GCCInstallation.isValid()) {
-    const auto &Callback = Multilibs.includeDirsCallback();
-    if (Callback) {
-      for (const auto &Path : Callback(GCCInstallation.getMultilib()))
-        addExternCSystemIncludeIfExists(
-            DriverArgs, CC1Args, GCCInstallation.getInstallPath() + Path);
-    }
-  }
+  AddMultilibIncludeArgs(DriverArgs, CC1Args);
 
   // Implement generic Debian multiarch support.
   const StringRef X86_64MultiarchIncludeDirs[] = {
       "/usr/include/x86_64-linux-gnu",
 
       // FIXME: These are older forms of multiarch. It's not clear that they're
       // in use in any released version of Debian, so we should consider
       // removing them.