[llvm-rc] Don't rewrite the arch in the default triple unless necessary

[mstorsjo]

Summary

When the default target arch isn't one that is supported as a
windows target, we want to set a suitable architecture (so that
Clang tests that run plain 'llvm-rc' succeed checks for e.g.
"#ifdef _WIN32" even for llvm builds that default to e.g. ppc64).

But if the default target architecture is usable, don't rewrite it.
(Rewriting it, by e.g. "T.setArch(T.getArch())", normalizes the
spelling of the architecture, e.g. changing i686 to i386. Such a
change can make clang unable to find the right sysroot.)

This can't, unfortunately, practically be tested very well because
it is entirely dependent on the default triple of the llvm build.

[jeremyd2019]

This solved https://github.com/msys2/CLANG-packages/issues/46 on msys2

[amccarth]

The idea looks fine. Just a couple inline questions.

llvm/tools/llvm-rc/llvm-rc.cpp

148

Should this be just isUsableArch? It doesn't seem like Default is relevant in this new formulation.

184

Is there a guarantee that the fallback will be usable?

[mstorsjo]

llvm/tools/llvm-rc/llvm-rc.cpp

148

Sure, I guess I could leave that word out

184

The main criterion for usable here practically is whether “clang -target <arch>-windows -E” works as expected, i.e. preprocesses with _WIN32 defined. Clang does handle that for those four arches even if the corresponding target lowering backend is disabled.

Or put another way, we’ve got tests within clang that try to do preprocessing by just calling llvm-rc or llvm-windres without specifying any target arch, and checking that the preprocessing works a as expected (defining _WIN32 and RC_DEFINED - the latter comes from a parameter passed from llvm-rc). If llvm defaults to e.g. ppc64, then we must override the arch and do the preprocessing test with e.g. an x86_64 triple.

[mstorsjo]

Renamed isUsableDefaultArch to isUsableArch

[mstorsjo]

@amccarth - Is this ok?

[amccarth]

LGTM. Thanks.

llvm/tools/llvm-rc/llvm-rc.cpp

ErrorOr<std::string> findClang(const char *Argv0) {
   for (const auto *Name : {"clang", "clang-cl"}) {
     Path = sys::findProgramByName(Name);
     if (Path)
       return Path;
   }
   return Path;
 }
 
-Triple::ArchType getDefaultArch(Triple::ArchType Arch) {
+bool isUsableDefaultArch(Triple::ArchType Arch) {
   switch (Arch) {
   case Triple::x86:
   case Triple::x86_64:
   case Triple::arm:
   case Triple::thumb:
   case Triple::aarch64:
     // These work properly with the clang driver, setting the expected
     // defines such as _WIN32 etc.
-    return Arch;
+    return true;
   default:
     // Other archs aren't set up for use with windows as target OS, (clang
-    // doesn't define e.g. _WIN32 etc), so set a reasonable default arch.
-    return Triple::x86_64;
+    // doesn't define e.g. _WIN32 etc), so with them we need to set a
+    // different default arch.
+    return false;
   }
 }
 
+Triple::ArchType getDefaultFallbackArch() {
+  return Triple::x86_64;
+}
+
 std::string getClangClTriple() {
   Triple T(sys::getDefaultTargetTriple());
-  T.setArch(getDefaultArch(T.getArch()));
+  if (!isUsableDefaultArch(T.getArch()))
+    T.setArch(getDefaultFallbackArch());
   T.setOS(Triple::Win32);
   T.setVendor(Triple::PC);
   T.setEnvironment(Triple::MSVC);
   T.setObjectFormat(Triple::COFF);
   return T.str();
 }
 
 std::string getMingwTriple() {
   Triple T(sys::getDefaultTargetTriple());
-  T.setArch(getDefaultArch(T.getArch()));
+  if (!isUsableDefaultArch(T.getArch()))
+    T.setArch(getDefaultFallbackArch());
   if (T.isWindowsGNUEnvironment())
     return T.str();
   // Write out the literal form of the vendor/env here, instead of
   // constructing them with enum values (which end up with them in
   // normalized form). The literal form of the triple can matter for
   // finding include files.
   return (Twine(T.getArchName()) + "-w64-mingw32").str();
 }