[PATCH] MinGW toolchain

[Saleem Abdulrasool]

================
Comment at: lib/Driver/MinGWToolChain.cpp:37
@@ +36,3 @@
+  const std::string GCClibdir =
+#ifdef _WIN32
+          getDriver().Dir + "/../lib/gcc/" + getTriple().getArchName().str() + "-w64-mingw32";
----------------
Please use LLVM_ON_WIN32 rather than the raw _WIN32 macro.

================
Comment at: lib/Driver/MinGWToolChain.cpp:41
@@ +40,3 @@
+          getDriver().SysRoot + "/usr/lib/gcc/" + getTriple().getArchName().str() + "-w64-mingw32";
+#endif
+  if (llvm::sys::fs::exists(GCClibdir)) {
----------------
Im not sure I understand this piece of the code.

It seems that you use the location of the driver and then move up to find the compiler library directory on Windows, but on Linux you use a sysroot relative path.

I am confused on the behaviour here since nothing prevents the use of --sysroot on Windows, so why not honour that always?  Uniformity in behaviour is probably a good thing here.

I was thinking something like this:

    const Driver &D = getDriver();
    const llvm::Triple &T = getTriple();
    Twine GCCLibDir = (D.SysRoot ? D.SysRoot + Twine("/usr")
                                 : llvm::sys::path::parent_path(D.Dir));
    GCCLibDir = GCCLibDir + "/lib/gcc" + T.getArchName() + "-w64-mingw32";

================
Comment at: lib/Driver/MinGWToolChain.cpp:51
@@ +50,3 @@
+
+#ifdef _WIN32
+  // assume sysrooted compiler
----------------
Similar to above, you could honour the sysroot parameter as well and default to the sibling library directory to the location of the driver.

================
Comment at: lib/Driver/MinGWToolChain.cpp:110
@@ +109,3 @@
+
+#ifdef _WIN32
+  // Clang runs on Windows, assume sysroot toolchain
----------------
Again here and in the next function.

================
Comment at: lib/Driver/Tools.h:621
@@ +620,3 @@
+  public:
+    Assemble(const ToolChain &TC) : Tool("dragonfly::Assemble", "assembler",
+                                         TC) {}
----------------
You probably want to change the dragonfly name here :-)

================
Comment at: lib/Driver/Tools.h:634
@@ +633,3 @@
+  public:
+    Link(const ToolChain &TC) : Tool("dragonfly::Link", "linker", TC) {}
+
----------------
Here as well.

================
Comment at: lib/Driver/Tools.cpp:7100
@@ +7099,3 @@
+
+  if (getToolChain().getArch() == llvm::Triple::x86) {
+    CmdArgs.push_back("--32");
----------------
Maybe add an assertion that only these architectures are ever received?  The curly braces are unnecessary.

================
Comment at: lib/Driver/Tools.cpp:7112
@@ +7111,3 @@
+
+  for (InputInfoList::const_iterator
+         it = Inputs.begin(), ie = Inputs.end(); it != ie; ++it) {
----------------
We have switched over to C++11, this might be clearer as:

    for (const auto &I : Inputs)
      CmdArgs.push_back(I.getFilename());

================
Comment at: lib/Driver/Tools.cpp:7130
@@ +7129,3 @@
+  const Driver &D = TC.getDriver();
+  //const SanitizerArgs &Sanitize = TC.getSanitizerArgs();
+
----------------
Why not just add a TODO comment and remove this line?

================
Comment at: lib/Driver/Tools.cpp:7152
@@ +7151,3 @@
+  if (TC.getArch() == llvm::Triple::x86_64)
+    CmdArgs.push_back("i386pep");
+
----------------
An assertion that only these two architectures ever reach here would be nice.

================
Comment at: lib/Driver/Tools.cpp:7163
@@ +7162,3 @@
+      if (TC.getArch() == llvm::Triple::x86)
+        CmdArgs.push_back("-e DllMainCRTStartup");
+      else
----------------
I may be mistaken here, but I believe you have the entry points swapped.

================
Comment at: lib/Driver/Tools.cpp:7179
@@ +7178,3 @@
+    else {
+    CmdArgs.push_back(Args.MakeArgString(TC.GetFilePath("crt2.o")));
+    CmdArgs.push_back(Args.MakeArgString(TC.GetFilePath("crtbegin.o")));
----------------
Is the indentation here off?

================
Comment at: lib/Driver/Tools.cpp:7188
@@ +7187,3 @@
+
+  for (ToolChain::path_list::const_iterator i = Paths.begin(), e = Paths.end();
+       i != e; ++i)
----------------
C++11 could make this nicer:

    for (const auto &P : Paths)
      CmdArgs.push_back(Args.MakeArgString(StringRef("-L") + P));

================
Comment at: lib/Driver/Tools.cpp:7194
@@ +7193,3 @@
+
+  //TODO: Add ASan stuff here
+
----------------
Space after the //.

================
Comment at: lib/Driver/Tools.cpp:7219
@@ +7218,3 @@
+
+      //TODO: remove unconditionally linking pthreads library
+      // Currently required for OpenMP and posix-threading libgcc
----------------
Space before the TODO.

================
Comment at: lib/Driver/Tools.cpp:7214
@@ +7213,3 @@
+
+      if (Args.hasArg(options::OPT_fopenmp))
+        CmdArgs.push_back("-lgomp");
----------------
Hmm...perhaps add a switch for the openmp framework and handle that (now that there is also the Intel OpenMP implementation.

================
Comment at: lib/Frontend/InitHeaderSearch.cpp:308
@@ -344,1 +307,3 @@
+  if ((os != llvm::Triple::RTEMS) &&
+     (!((os == llvm::Triple::Win32) && (triple.getEnvironment() == llvm::Triple::GNU))))
     AddPath("/usr/include", ExternCSystem, false);
----------------
This can be simplified a bit as:

    if (os != llvm::Triple::RTEMS && !Triple.isWindowsGNUEnvironment())


http://reviews.llvm.org/D3420