[Driver] Move description of toolchain specific include directories from the common driver code to the corresponding `MultilibSet` declarations

[atanasyan]

Summary

The patch moves description of toolchain specific include directories from the common driver code to the corresponding MultilibSet declarations.

The MultilibSet can hold an optional callback function which is responsible to return a set of include directories specific for the toolchain. That allows, for example, to remove MIPS toolchain specific directories from Linux::AddClangSystemIncludeArgs method and simplify adding new directories in the future.

[atanasyan]

Ping?

[• rafael]

The code change seems reasonable if the flexibility of a callback is needed. I have no opinion on whether it is.

Do you have the followup change where this is used?

include/clang/Driver/Multilib.h

158

setincludeDirsCallback would be the correct name, no?

[atanasyan]

Yes, I am going to add more code into these callback in the next patch. See http://reviews.llvm.org/D4790.

include/clang/Driver/Multilib.h

158

OK

[• rafael]

There is a functionality change in that CSMipsMultilibs will not search InstallDir + "/../../../../sysroot/usr/include". Is that expected?

lib/Driver/ToolChains.cpp

1838

This is identical to the first lambda, no? Can you factor it out?

[atanasyan]

In fact CSMipsMultilibs and FSFMipsMultilibs toolchains require different include paths. Currently we have to probe all possible paths in the Linux::AddClangSystemIncludeArgs() routine. One of the goal of this patch is to get paths from the toolchain description and do not enumerate all possible variants. So it is intended change.

lib/Driver/ToolChains.cpp

1838

Yes, both lambdas are identical now but it is just a chance. The corresponding toolchains are not related, have separate directories trees in the tests etc. So I think the factoring out might make the code more complicated and error prone.

[• rafael]

LGTM then.

[atanasyan]

Thanks for review.

Closed by commit rL214949.

include/clang/Driver/Multilib.h

 raw_ostream &operator<<(raw_ostream &OS, const Multilib &M);
 
 class MultilibSet {
 public:
   typedef std::vector<Multilib> multilib_list;
   typedef multilib_list::iterator iterator;
   typedef multilib_list::const_iterator const_iterator;
 
+  typedef std::function<std::vector<std::string>(
+      StringRef InstallDir, StringRef Triple, const Multilib &M)>
+  IncludeDirsFunc;
+
   struct FilterCallback {
     virtual ~FilterCallback() {};
     /// \return true iff the filter should remove the Multilib from the set
     virtual bool operator()(const Multilib &M) const = 0;
   };
 
 private:
   multilib_list Multilibs;
+  IncludeDirsFunc IncludeCallback;
 
 public:
   MultilibSet() {}
 
   /// Add an optional Multilib segment
   MultilibSet &Maybe(const Multilib &M);
 
   /// Add a set of mutually incompatible Multilib segments
public:
 
   /// Pick the best multilib in the set, \returns false if none are compatible
   bool select(const Multilib::flags_list &Flags, Multilib &M) const;
 
   unsigned size() const { return Multilibs.size(); }
 
   void print(raw_ostream &OS) const;
 
+  MultilibSet &includeDirsCallback(IncludeDirsFunc F) {
+    IncludeCallback = F;
+    return *this;
+  }
+  IncludeDirsFunc includeDirsCallback() const { return IncludeCallback; }
+
 private:
   /// Apply the filter to Multilibs and return the subset that remains
   static multilib_list filterCopy(const FilterCallback &F,
                                   const multilib_list &Ms);
 
   /// Apply the filter to the multilib_list, removing those that don't match
   static void filterInPlace(const FilterCallback &F, multilib_list &Ms);
 };
 
 raw_ostream &operator<<(raw_ostream &OS, const MultilibSet &MS);
 }
 }
 
 #endif
 

lib/Driver/ToolChains.cpp

FSFMipsMultilibs = MultilibSet()
       .FilterOut("/micromips/64")
       .FilterOut("/mips32/64")
       .FilterOut("^/64")
       .FilterOut("/mips16/64")
       .Either(BigEndian, LittleEndian)
       .Maybe(SoftFloat)
       .Maybe(Nan2008)
       .FilterOut(".*sof/nan2008")
-      .FilterOut(NonExistent);
+      .FilterOut(NonExistent)
+      .includeDirsCallback([](
+          StringRef InstallDir, StringRef TripleStr, const Multilib &M) {
+        std::vector<std::string> Dirs;
+        Dirs.push_back((InstallDir + "/include").str());
+        Dirs.push_back((InstallDir + "/../../../../sysroot/usr/include").str());
+        return Dirs;
+      });
   }
 
   // Check for Code Sourcery toolchain multilibs
   MultilibSet CSMipsMultilibs;
   {
     auto MArchMips16 = makeMultilib("/mips16")
       .flag("+m32").flag("+mips16");
 
CSMipsMultilibs = MultilibSet()
       .Either(MArchMips16, MArchMicroMips, MArchDefault)
       .Either(SoftFloat, Nan2008, DefaultFloat)
       .FilterOut("/micromips/nan2008")
       .FilterOut("/mips16/nan2008")
       .Either(BigEndian, LittleEndian)
       .Maybe(MAbi64)
       .FilterOut("/mips16.*/64")
       .FilterOut("/micromips.*/64")
-      .FilterOut(NonExistent);
+      .FilterOut(NonExistent)
+      .includeDirsCallback([](
+          StringRef InstallDir, StringRef TripleStr, const Multilib &M) {
+        std::vector<std::string> Dirs;
+        Dirs.push_back((InstallDir + "/include").str());
+        Dirs.push_back((InstallDir + "/../../../../" + TripleStr +
+                        "/libc/usr/include").str());
+        return Dirs;
+      });
   }
 
   MultilibSet AndroidMipsMultilibs = MultilibSet()
     .Maybe(Multilib("/mips-r2").flag("+march=mips32r2"))
     .FilterOut(NonExistent);
 
   MultilibSet DebianMipsMultilibs;
   {
MultilibSet ImgMultilibs;
 
     auto MAbi64 = makeMultilib("/64")
       .flag("+mabi=n64").flag("-mabi=n32").flag("-m32");
 
     ImgMultilibs = MultilibSet()
       .Maybe(Mips64r6)
       .Maybe(MAbi64)
       .Maybe(LittleEndian)
-      .FilterOut(NonExistent);
+      .FilterOut(NonExistent)
+      .includeDirsCallback([](
+          StringRef InstallDir, StringRef TripleStr, const Multilib &M) {
+        std::vector<std::string> Dirs;
+        Dirs.push_back((InstallDir + "/include").str());
+        Dirs.push_back((InstallDir + "/../../../../sysroot/usr/include").str());
+        return Dirs;
+      });
   }
 
   StringRef CPUName;
   StringRef ABIName;
   tools::mips::getMipsCPUAndABI(Args, TargetTriple, CPUName, ABIName);
 
   llvm::Triple::ArchType TargetArch = TargetTriple.getArch();
 
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.
 
-  // Sourcery CodeBench and modern FSF Mips toolchains put extern C
-  // system includes under three additional directories.
-  if (GCCInstallation.isValid() && isMipsArch(getTriple().getArch())) {
-    addExternCSystemIncludeIfExists(
-        DriverArgs, CC1Args, GCCInstallation.getInstallPath() + "/include");
-
-    addExternCSystemIncludeIfExists(
-        DriverArgs, CC1Args,
-        GCCInstallation.getInstallPath() + "/../../../../" +
-            GCCInstallation.getTriple().str() + "/libc/usr/include");
-
-    addExternCSystemIncludeIfExists(
-        DriverArgs, CC1Args,
-        GCCInstallation.getInstallPath() + "/../../../../sysroot/usr/include");
+  // Add include directories specific to the selected multilib set and multilib.
+  if (GCCInstallation.isValid()) {
+    auto Callback = Multilibs.includeDirsCallback();
+    if (Callback) {
+      const auto IncludePaths = Callback(GCCInstallation.getInstallPath(),
+                                         GCCInstallation.getTriple().str(),
+                                         GCCInstallation.getMultilib());
+      for (const auto &Path : IncludePaths)
+        addExternCSystemIncludeIfExists(DriverArgs, CC1Args, Path);
+    }
   }
 
   // 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