[PATCH] Add Multilib selection machinery to Clang

[Simon Atanasyan]

  Please update the patch against the current Clang source code.


================
Comment at: lib/Driver/ArgParse.h:1
@@ +1,2 @@
+//===--- ArgParse.h - Argument Parsing Utilities ----------------*- C++ -*-===//
+//
----------------
Jon Roelofs wrote:
> Simon Atanasyan wrote:
> > Is it really necessary to move all these functions to the separate h/cpp files in this commit?
> A few of these would end up being duplicated if I left them as static functions. Would it be better to do this in a separate commit?  Do you have a better suggestion for the name of this file? (I don't really like 'ArgParse', but it's the best I could come up with).
As far as I can see all these functions except `hasMipsN32ABIArg` are used in the single file. Let's keep these function where they are used. As to `hasMipsN32ABIArg` we can declare it in the `Tools.h` file. There are `namespace arm` and `namespace hexagon` in this file. Let's create `mips` namespace and put the declaration to it.

================
Comment at: include/clang/Driver/Multilib.h:83
@@ +82,3 @@
+
+  struct FilterCallback {
+    /// \return true iff the filter should remove the Multilib from the set
----------------
The Clang shows the following warning here:
  'clang::driver::MultilibSet::FilterCallback' has virtual functions but non-virtual destructor

================
Comment at: lib/Driver/Multilib.cpp:1
@@ +1,2 @@
+//===--- Multilib.cpp - Multilib Implementation ---------------------------===//
+//
----------------
Jon Roelofs wrote:
> Simon Atanasyan wrote:
> > We need test cases cover the `Multilib` functionality.
> Like the ones in clang/unittests or clang/test? Both?
I thought about new test in clang/test/Driver. But probably it's better to create a unit test for this class.

================
Comment at: lib/Driver/Multilib.cpp:175
@@ +174,3 @@
+static Multilib compose(const Multilib &Base, const Multilib &New) {
+  Multilib Composed(Base.gccSuffix() + New.gccSuffix(),
+                    Base.osSuffix() + New.osSuffix(),
----------------
Jon Roelofs wrote:
> Simon Atanasyan wrote:
> > It's potentially dangerous to join two string represent arbitrary paths. Can we trust that we never get double slashes `//` and no one slash at all at the point of joining?
> Should I add assertions? Would it be better to use llvm/Support/Path.h for these suffixes?
If there is some sort of a contract that say base suffix never ends by a slash and new suffix always starts by a slash we can put asserts here. If not it's better to try to use `llvm::sys::path` here.

================
Comment at: lib/Driver/ToolChains.cpp:1563
@@ +1562,3 @@
+    Multilibs.combineWith(AndroidMipsMultilibs);
+  else if (DebianMipsMultilibs.size() == 3) {
+    Multilibs.combineWith(DebianMipsMultilibs);
----------------
Jon Roelofs wrote:
> Simon Atanasyan wrote:
> > Magical code on this line and below. Why do we compare `DebianMipsMultilibs.size()` and 3? Why do we combine `Multilibs` and `FSFMipsMultilibs` if `FSFMipsMultilibs.size() > CSMipsMultilibs.size()` etc?
> Yes, this is a bit of 'Magic', but I'm not sure of a better solution. The problem here is that each of the MultilibSets overlap a little, so you get partial matches when looking at one installation and comparing it with the 'wrong' MultilibSet.  I could add a comment explaining this, or maybe you a have a suggestion for a better approach :) ?
Probably a comment would be enough.


http://llvm-reviews.chandlerc.com/D2538