Driver: Better detection of mingw-gcc

[martell]

Summary

As discussed after r253898 here is the better gcc detection.

[martell]

Removed the break that I didn't think was needed
Can be re-added ??

Will added testcases

[martell]

@ismail can you test this for your setup please? :)

[yaron.keren]

This always searches for something-gcc and then discards the result if sysroot was provided, which is a waste.
Move the searching to a helper function and then it can be done only if sysroot was not provided, as it is now.

The break is needed to avoid looking for mingw32-gcc after x86_64-w64-mingw32-gcc was already found. It's a wasted search.

[martell]

Updated to reflect feedback

I'm not too sure what todo for testcases on this one because they already exist for the most part.
I ran the test suite and they all pass.

I would have to create a mingw32-gcc and {armv7|i686|x86_64}-w64-mingw32-gcc and chmod+x for the testcase.
Is that safe ?

[ismail]

Tested on openSUSE and it works. Thanks!

[yaron.keren]

findGccDir() can return llvm::ErrorOr<std::string> and then all Base assignments happen at the same if-elseif-else:

if (getDriver().SysRoot.size())
  Base = getDriver().SysRoot;
else if (llvm::ErrorOr<std::string> GPPName = findGccDir())
 Base = llvm::sys::path::parent_path(
        llvm::sys::path::parent_path(GPPName.get()));
else
  Base = llvm::sys::path::parent_path(getDriver().getInstalledDir());

Then, findGccDir() is actually findGcc().

About tests, adding empty script "gcc" with x set in the directory structure, and adding the directory to the path at start of the LIT test may work. It will fail on Windows so try this with a new test file so we can XFAIL:windows only the new one if required.

[ismail]

Martell, any update on this?

[martell]

hi ismail,
I will tidy up for review closer towards the weekend where i can work on out of work stuff

[martell]

Sorry I was a bit slow getting around to this.

Will do testcases if Yaron thinks this is okay

[yaron.keren]

LGTM with some tests cases.

[ismail]

LLVM 3.8 will be branched in a week, if we can't get this in before then I'll revert r253898 before branching and apply it again after branching done.

[ismail]

Reverted in r257468.

[martell]

After the revert I left this patch go to pick up some other patch work.
Seen as we are not anywhere near the next release this seem like a good time to pick it up again.
Also got a reminder when yaron resigned

Updated to master.
@ismail can you confirm this update works for your custom opensuse setup on master.
@yaron.keren I can't do a non flaky test case for this it seems because on windows we need .exe extension and not on unix.
Also we need the test case to be able to set the PATH variable to be able to test this fully.
I can't find any example that lets us set the path variable? Help welcome :)

I would like to consider this as working because it passes all the existing Driver testcases in test/Driver/mingw.cpp

[martell]

To clarify the most import part about actually being able to test this is the being able to modify the PATH variable for windows and unix hosts in the test case.
The program prefix is less of a problem because I can double down with the same empty file one with .exe and one without.
Just thought that I would note it incase someone has ran into this already.

[yaron.keren]

Adding Mateusz and Reid.

[mati865]

Looks fine for MinGW on Windows.

[martell]

Committed as rL300555

lib/Driver/ToolChains/MinGW.h

private:
 
   std::string Base;
   std::string GccLibDir;
   std::string Ver;
   std::string Arch;
   mutable std::unique_ptr<tools::gcc::Preprocessor> Preprocessor;
   mutable std::unique_ptr<tools::gcc::Compiler> Compiler;
   void findGccLibDir();
+  llvm::ErrorOr<std::string> findGcc();
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_MINGW_H

lib/Driver/ToolChains/MinGW.cpp

for (StringRef CandidateArch : Archs) {
       if (findGccVersion(LibDir, GccLibDir, Ver)) {
         Arch = CandidateArch;
         return;
       }
     }
   }
 }
 
+llvm::ErrorOr<std::string> toolchains::MinGW::findGcc() {
+  llvm::SmallVector<llvm::SmallString<32>, 2> Gccs;
+  Gccs.emplace_back(getTriple().getArchName());
+  Gccs[0] += "-w64-mingw32-gcc";
+  Gccs.emplace_back("mingw32-gcc");
+  // Please do not add "gcc" here
+  for (StringRef CandidateGcc : Gccs)
+    if (llvm::ErrorOr<std::string> GPPName = llvm::sys::findProgramByName(CandidateGcc))
+      return GPPName;
+  return make_error_code(std::errc::no_such_file_or_directory);
+}
+
 toolchains::MinGW::MinGW(const Driver &D, const llvm::Triple &Triple,
                          const ArgList &Args)
     : ToolChain(D, Triple, Args), CudaInstallation(D, Triple, Args) {
   getProgramPaths().push_back(getDriver().getInstalledDir());
 
-// In Windows there aren't any standard install locations, we search
-// for gcc on the PATH. In Linux the base is always /usr.
-#ifdef LLVM_ON_WIN32
   if (getDriver().SysRoot.size())
     Base = getDriver().SysRoot;
-  else if (llvm::ErrorOr<std::string> GPPName =
-               llvm::sys::findProgramByName("gcc"))
+  else if (llvm::ErrorOr<std::string> GPPName = findGcc())
     Base = llvm::sys::path::parent_path(
         llvm::sys::path::parent_path(GPPName.get()));
   else
     Base = llvm::sys::path::parent_path(getDriver().getInstalledDir());
-#else
-  if (getDriver().SysRoot.size())
-    Base = getDriver().SysRoot;
-  else
-    Base = "/usr";
-#endif
 
   Base += llvm::sys::path::get_separator();
   findGccLibDir();
   // GccLibDir must precede Base/lib so that the
   // correct crtbegin.o ,cetend.o would be found.
   getFilePaths().push_back(GccLibDir);
   getFilePaths().push_back(
       (Base + Arch + llvm::sys::path::get_separator() + "lib").str());