[clang] [MinGW] Guess the right ix86 arch name spelling as sysroot

[mstorsjo]

Summary

For x86, most contempory mingw toolchains use i686 as 32 bit
x86 arch target.

As long as the target triple is set to the right form, this works
fine, either as the compiler's default target, or via e.g.
a triple prefix like i686-w64-mingw32-clang.

However, if the unprefixed toolchain targets x86_64, but the user
tries to switch it to target 32 bit by adding the -m32 option, the
computeTargetTriple function in Clang, together with
Triple::get32BitArchVariant, sets the arch to i386. This causes
the right sysroot to not be found.

When targeting an arch where there are potential spelling ambiguities
with respect to the sysroots (i386 and arm), check if the driver can
find a sysroot with the arch name - if not, try a couple other
candidates.

Other design ideas considered: It would fit in better with the design
to do this kind of arch name fixups in computeTargetTriple (it
already has a couple other OS/arch specific cases), but the
heuristics for detecting the potential right choice ties in quite
closely with how the toolchain looks for the implicit sysroot to use,
so I'd prefer to keep that logic in the toolchain::MinGW class/file.

This is done as a wrapper function before invoking the toolchain::MinGW()
constructor, because the MinGW() constructor can't run code of its own
until the superclass ToolChain() constructor returns. The fixups need to
be done before the ToolChain() constructor, because it uses the Triple
for per-target runtime directories.

[mstorsjo]

I guess a downside of this solution, is that if an i686 sysroot exists next to the clang binary, it becomes practically impossible to test codegen differences when you run it with various -target iX86-w64-mingw32 options, as they'd all be corrected back to i686. (This wouldn't be an issue if the autodetection was done in computeTargetTriple only when the -m32/-m64 options are used though.)

[mstorsjo]

In D111952#3068680, @mstorsjo wrote:

I guess a downside of this solution, is that if an i686 sysroot exists next to the clang binary, it becomes practically impossible to test codegen differences when you run it with various -target iX86-w64-mingw32 options, as they'd all be corrected back to i686. (This wouldn't be an issue if the autodetection was done in computeTargetTriple only when the -m32/-m64 options are used though.)

Ok, it turned out to not be that bad to move the fixup into computeTargetTriple to this location after all, it looks fairly neat and not that much out of line compared to the existing triple tweaks done there.

[mstorsjo]

Moved the fixup into computeTargetTriple, so it's only done after applying the -m32/-m64 options.

[mstorsjo]

Rebased after renaming SysrootName to SubdirName.

[MaskRay]

clang/lib/Driver/ToolChains/MinGW.h

63

Use functionName for newer functions. (It's unfortunate that Clang has much inconsistency but we should do the right thing for new methods.)

clang/test/Driver/mingw-sysroot.cpp

49

Changing PATH seems unreliable for some test environments.

Any alternative way to test this?

--target= is preferred over -target for driver options. -target is legacy but we probably need to keep indefinitely for its wide usage.

linux-cross.cpp has some nice tests which may inspire this file ? :)

[mstorsjo]

Fixed the naming of the new function, using --target= in the newly added test lines, fixed a case of missed clang-format.

[mstorsjo]

clang/lib/Driver/ToolChains/MinGW.h

63

Sure, will change it.

clang/test/Driver/mingw-sysroot.cpp

49

Using PATH isn't very nice, no, but it's at least reliable enough to run on the current testing setups?

I don't see any good input for testing this in linux-cross.cpp - that file uses explicit --sysroot= parameters all over the place, while this test file is mostly about how to best implicitly deduce the sysroot when one isn't specified.

This file (both the old and the newly added tests) relies on checking what directories it finds in <clang-executable>/../<triple>, so it requires fake-installing the clang binary by symlinking into a test toolchain tree.

I guess it could be possible to execute that fake-installed clang binary by specifying the direct path to it, instead of adding it to PATH - but it's an important usecase to make sure that it works when the literal path isn't spelled out in argv[0] too.

I can change to use --target= in the added lines, but the rest of the file uses the other spelling anyway (and I don't think it's on topic to do such rewrites as part of this patch).

clang/lib/Driver/Driver.cpp

if (A->getOption().matches(options::OPT_m64)) {
       else if (Target.getEnvironment() == llvm::Triple::MuslX32)
         Target.setEnvironment(llvm::Triple::Musl);
     } else if (A->getOption().matches(options::OPT_m16) &&
                Target.get32BitArchVariant().getArch() == llvm::Triple::x86) {
       AT = llvm::Triple::x86;
       Target.setEnvironment(llvm::Triple::CODE16);
     }
 
-    if (AT != llvm::Triple::UnknownArch && AT != Target.getArch())
+    if (AT != llvm::Triple::UnknownArch && AT != Target.getArch()) {
       Target.setArch(AT);
+      if (Target.isWindowsGNUEnvironment())
+        toolchains::MinGW::fixTripleArch(D, Target, Args);
+    }
   }
 
   // Handle -miamcu flag.
   if (Args.hasFlag(options::OPT_miamcu, options::OPT_mno_iamcu, false)) {
     if (Target.get32BitArchVariant().getArch() != llvm::Triple::x86)
       D.Diag(diag::err_drv_unsupported_opt_for_target) << "-miamcu"
                                                        << Target.str();
 

clang/lib/Driver/ToolChains/MinGW.h

 
 namespace toolchains {
 
 class LLVM_LIBRARY_VISIBILITY MinGW : public ToolChain {
 public:
   MinGW(const Driver &D, const llvm::Triple &Triple,
         const llvm::opt::ArgList &Args);
 
+  static void fixTripleArch(const Driver &D, llvm::Triple &Triple,
+                            const llvm::opt::ArgList &Args);
+
   bool HasNativeLLVMSupport() const override;
 
   bool IsIntegratedAssemblerDefault() const override;
   bool IsUnwindTablesDefault(const llvm::opt::ArgList &Args) const override;
   bool isPICDefault() const override;
   bool isPIEDefault() const override;
   bool isPICDefaultForced() const override;
 
private:
 
   std::string Base;
   std::string GccLibDir;
   std::string Ver;
   std::string SubdirName;
   mutable std::unique_ptr<tools::gcc::Preprocessor> Preprocessor;
   mutable std::unique_ptr<tools::gcc::Compiler> Compiler;
   void findGccLibDir();
-  llvm::ErrorOr<std::string> findGcc();
-  llvm::ErrorOr<std::string> findClangRelativeSysroot();
 
   bool NativeLLVMSupport;
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_MINGW_H

clang/lib/Driver/ToolChains/MinGW.cpp

for (StringRef CandidateSysroot : SubdirNames) {
       if (findGccVersion(LibDir, GccLibDir, Ver)) {
         SubdirName = std::string(CandidateSysroot);
         return;
       }
     }
   }
 }
 
-llvm::ErrorOr<std::string> toolchains::MinGW::findGcc() {
+static llvm::ErrorOr<std::string> findGcc(const llvm::Triple &T) {
   llvm::SmallVector<llvm::SmallString<32>, 2> Gccs;
-  Gccs.emplace_back(getTriple().getArchName());
+  Gccs.emplace_back(T.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);
 }
 
-llvm::ErrorOr<std::string> toolchains::MinGW::findClangRelativeSysroot() {
+static llvm::ErrorOr<std::string>
+findClangRelativeSysroot(const Driver &D, const llvm::Triple &T,
+                         std::string &SubdirName) {
   llvm::SmallVector<llvm::SmallString<32>, 2> Subdirs;
-  Subdirs.emplace_back(getTriple().str());
-  Subdirs.emplace_back(getTriple().getArchName());
+  Subdirs.emplace_back(T.str());
+  Subdirs.emplace_back(T.getArchName());
   Subdirs[1] += "-w64-mingw32";
-  StringRef ClangRoot =
-      llvm::sys::path::parent_path(getDriver().getInstalledDir());
+  StringRef ClangRoot = llvm::sys::path::parent_path(D.getInstalledDir());
   StringRef Sep = llvm::sys::path::get_separator();
   for (StringRef CandidateSubdir : Subdirs) {
     if (llvm::sys::fs::is_directory(ClangRoot + Sep + CandidateSubdir)) {
       SubdirName = std::string(CandidateSubdir);
       return (ClangRoot + Sep + CandidateSubdir).str();
     }
   }
   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),
       RocmInstallation(D, Triple, Args) {
   getProgramPaths().push_back(getDriver().getInstalledDir());
 
+  // The sequence for detecting a sysroot here should be kept in sync with
+  // the testTriple function below.
   if (getDriver().SysRoot.size())
     Base = getDriver().SysRoot;
   // Look for <clang-bin>/../<triplet>; if found, use <clang-bin>/.. as the
   // base as it could still be a base for a gcc setup with libgcc.
-  else if (llvm::ErrorOr<std::string> TargetSubdir = findClangRelativeSysroot())
+  else if (llvm::ErrorOr<std::string> TargetSubdir =
+               findClangRelativeSysroot(getDriver(), getTriple(), SubdirName))
     Base = std::string(llvm::sys::path::parent_path(TargetSubdir.get()));
-  else if (llvm::ErrorOr<std::string> GPPName = findGcc())
+  else if (llvm::ErrorOr<std::string> GPPName = findGcc(getTriple()))
     Base = std::string(llvm::sys::path::parent_path(
         llvm::sys::path::parent_path(GPPName.get())));
   else
     Base = std::string(
         llvm::sys::path::parent_path(getDriver().getInstalledDir()));
 
   Base += llvm::sys::path::get_separator();
   findGccLibDir();
for (auto &CppIncludeBase : CppIncludeBases) {
       addSystemInclude(DriverArgs, CC1Args, CppIncludeBase);
       CppIncludeBase += Slash;
       addSystemInclude(DriverArgs, CC1Args, CppIncludeBase + SubdirName);
       addSystemInclude(DriverArgs, CC1Args, CppIncludeBase + "backward");
     }
     break;
   }
 }
+
+static bool testTriple(const Driver &D, const llvm::Triple &Triple,
+                       const ArgList &Args) {
+  // If an explicit sysroot is set, that will be used and we shouldn't try to
+  // detect anything else.
+  std::string SubdirName;
+  if (D.SysRoot.size())
+    return true;
+  if (llvm::ErrorOr<std::string> TargetSubdir =
+          findClangRelativeSysroot(D, Triple, SubdirName))
+    return true;
+  if (llvm::ErrorOr<std::string> GPPName = findGcc(Triple))
+    return true;
+  // If we neither found a colocated sysroot or a matching gcc executable,
+  // conclude that we can't know if this is the correct spelling of the triple.
+  return false;
+}
+
+static llvm::Triple adjustTriple(const Driver &D, const llvm::Triple &Triple,
+                                 const ArgList &Args) {
+  // First test if the original triple can find a sysroot with the triple
+  // name.
+  if (testTriple(D, Triple, Args))
+    return Triple;
+  llvm::SmallVector<llvm::StringRef, 3> Archs;
+  // If not, test a couple other possible arch names that might be what was
+  // intended.
+  if (Triple.getArch() == llvm::Triple::x86) {
+    Archs.emplace_back("i386");
+    Archs.emplace_back("i586");
+    Archs.emplace_back("i686");
+  } else if (Triple.getArch() == llvm::Triple::arm ||
+             Triple.getArch() == llvm::Triple::thumb) {
+    Archs.emplace_back("armv7");
+  }
+  for (auto A : Archs) {
+    llvm::Triple TestTriple(Triple);
+    TestTriple.setArchName(A);
+    if (testTriple(D, TestTriple, Args))
+      return TestTriple;
+  }
+  // If none was found, just proceed with the original value.
+  return Triple;
+}
+
+void toolchains::MinGW::fixTripleArch(const Driver &D, llvm::Triple &Triple,
+                                      const ArgList &Args) {
+  if (Triple.getArch() == llvm::Triple::x86 ||
+      Triple.getArch() == llvm::Triple::arm ||
+      Triple.getArch() == llvm::Triple::thumb)
+    Triple = adjustTriple(D, Triple, Args);
+}

clang/test/Driver/mingw-sysroot.cpp

 // REQUIRES: shell
 // UNSUPPORTED: system-windows
 
 // RUN: rm -rf %T/testroot-gcc
 // RUN: mkdir -p %T/testroot-gcc/bin
 // RUN: ln -s %clang %T/testroot-gcc/bin/x86_64-w64-mingw32-gcc
 // RUN: ln -s %clang %T/testroot-gcc/bin/x86_64-w64-mingw32-clang
 // RUN: ln -s %S/Inputs/mingw_ubuntu_posix_tree/usr/x86_64-w64-mingw32 %T/testroot-gcc/x86_64-w64-mingw32
 // RUN: ln -s %S/Inputs/mingw_ubuntu_posix_tree/usr/lib %T/testroot-gcc/lib
 
 // RUN: rm -rf %T/testroot-clang
 // RUN: mkdir -p %T/testroot-clang/bin
 // RUN: ln -s %clang %T/testroot-clang/bin/x86_64-w64-mingw32-clang
 // RUN: ln -s %S/Inputs/mingw_ubuntu_posix_tree/usr/x86_64-w64-mingw32 %T/testroot-clang/x86_64-w64-mingw32
+// RUN: ln -s %S/Inputs/mingw_arch_tree/usr/i686-w64-mingw32 %T/testroot-clang/i686-w64-mingw32
 
 
 // If we find a gcc in the path with the right triplet prefix, pick that as
 // sysroot:
 
 // RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %clang -target x86_64-w64-mingw32 -rtlib=platform -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_GCC %s
 // CHECK_TESTROOT_GCC: "-internal-isystem" "[[BASE:[^"]+]]/testroot-gcc{{/|\\\\}}lib{{/|\\\\}}gcc{{/|\\\\}}x86_64-w64-mingw32{{/|\\\\}}10.2-posix{{/|\\\\}}include{{/|\\\\}}c++"
 // CHECK_TESTROOT_GCC-SAME: {{^}} "-internal-isystem" "[[BASE]]/testroot-gcc{{/|\\\\}}lib{{/|\\\\}}gcc{{/|\\\\}}x86_64-w64-mingw32{{/|\\\\}}10.2-posix{{/|\\\\}}include{{/|\\\\}}c++{{/|\\\\}}x86_64-w64-mingw32"
 // CHECK_TESTROOT_GCC-SAME: {{^}} "-internal-isystem" "[[BASE]]/testroot-gcc{{/|\\\\}}lib{{/|\\\\}}gcc{{/|\\\\}}x86_64-w64-mingw32{{/|\\\\}}10.2-posix{{/|\\\\}}include{{/|\\\\}}c++{{/|\\\\}}backward"
 // CHECK_TESTROOT_GCC: "-internal-isystem" "[[BASE]]/testroot-gcc{{/|\\\\}}x86_64-w64-mingw32{{/|\\\\}}include"
 
 
 // If there's a matching sysroot next to the clang binary itself, prefer that
 // over a gcc in the path:
 
 // RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang/bin/x86_64-w64-mingw32-clang -target x86_64-w64-mingw32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG %s
 // CHECK_TESTROOT_CLANG: "{{[^"]+}}/testroot-clang{{/|\\\\}}x86_64-w64-mingw32{{/|\\\\}}include"
 
 
 // If we pick a root based on a sysroot next to the clang binary, which also
 // happens to be in the same directory as gcc, make sure we still can pick up
 // the libgcc directory:
 
 // RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-gcc/bin/x86_64-w64-mingw32-clang -target x86_64-w64-mingw32 -rtlib=platform -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_GCC %s
+
+
+// If the user requests a different arch via the -m32 option, which changes
+// x86_64 into i386, check that the driver notices that it can't find a
+// sysroot for i386 but there is one for i686, and uses that one.
+// (In practice, the real usecase is when using an unprefixed native clang
+// that defaults to x86_64 mingw, but it's easier to test this in cross setups
+// with symlinks, like the other tests here.)
+
+// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%" %T/testroot-clang/bin/x86_64-w64-mingw32-clang --target=x86_64-w64-mingw32 -m32 -rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck -check-prefix=CHECK_TESTROOT_CLANG_I686 %s
+// CHECK_TESTROOT_CLANG_I686: "{{[^"]+}}/testroot-clang{{/|\\\\}}i686-w64-mingw32{{/|\\\\}}include"