Refactor how the MSVC toolchain searches for a compatibility version.

[dlj]

Summary

The MSVC toolchain and Clang driver combination currently uses a fairly complex
sequence of steps to determine the MS compatibility version to pass to cc1.
There is some oddness in this sequence currently, with some code which inspects
flags in the toolchain, and some code which inspects the triple and local
environment in the driver code.

This change is an attempt to consolidate most of this logic so that
Win32-specific code lives in MSVCToolChain.cpp. I'm not 100% happy with the
split, so any suggestions are welcome.

There are a few things you might want to watch for for specifically:

• On all platforms, if MSVC compatibility flags are provided (and valid), use those.
• The fallback sequence should be the same as before, but is now consolidated into MSVCToolChain::getMSVCVersion:
  • Otherwise, try to use the Triple.
  • Otherwise, on Windows, check the executable.
  • Otherwise, on Windows or with --fms-extensions, default to 18.
  • Otherwise, we can't determine the version.
• MSVCToolChain::ComputeEffectiveTriple no longer calls the base ToolChain::ComputeEffectiveClangTriple. The only thing it would change for Windows the architecture, which we don't care about for the compatibility version.
  • I'm not sure whether this is philosophically correct (but it should be easy to add back to MSVCToolChain::getMSVCVersionFromTriple if not).
  • Previously, Tools.cpp just called getTriple() anyhow, so it doesn't look like the effective triple was always being used previously anyhow.

[dlj]

FYI to reviewers: I talked to Reid about this and sent this mostly for preliminary comments. If this approach seems OK, I'll flesh out some of the testing first, to ensure this doesn't break any corner cases.

[rnk]

This seems like a nice refactoring, and I'm pretty sure there's no real functional change in practice.

include/clang/Driver/ToolChain.h

446 ↗

(On Diff #80461)

This isn't a cheap operation, and we don't cache it. Let's call it something like computeMSVCVersion. I don't want us to read the version out of cl.exe more than once. Besides, it mirrors ComputeLLVMTriple etc.

lib/Driver/MSVCToolChain.cpp

476–478 ↗

(On Diff #80461)

I know this is more of an RFC patch, but I'll note we prefer static on free functions in case you hadn't encountered that http://llvm.org/docs/CodingStandards.html#id53

Also, this is poorly named. It's really building a version tuple from an unseparated, possibly full, version number. So, maybe inferMSVCVersionSeparators or separateMSVCFullVersion or something?

lib/Driver/Tools.cpp

5739 ↗

(On Diff #80461)

This comment got separated from the the relevant default some time ago. Can you move it to the return VersionTuple(18)?

[amccarth]

Nice refactor. I've been knee-deep in this code before, and it looks right to me.

[dlj]

Addressed comments, moved code for failing tests I couldn't figure out before.

[dlj]

Address more comments.

[dlj]

As it turns out, the flags are needed for non-MSVC toolchains as well (which was confusing me with broken tests). I've moved some of the args logic out of MSVCToolChain.cpp to ToolChain.cpp.

[rnk]

Can you update the commit message to outline the new algorithm? I think it's the same, except on all platforms, -fmsc-version and -fms-compatibility version are considered. Only with the MSVC toolchain do we go to the triple step, then the exe, then default to 18 with -fms-extensions. I guess this lets people compile code for Linux while pretending to be MSVC.

lib/Driver/MSVCToolChain.cpp

476 ↗

(On Diff #80526)

This function isn't doing the separation. Whatever code does the repeated mod 100 division is the separation code. This is more getMSVCVersionFromTriple.

lib/Driver/ToolChain.cpp

725 ↗

(On Diff #80526)

This thing is separateMSVCFullVersion.

[dlj]

In D27477#615983, @rnk wrote:

Can you update the commit message to outline the new algorithm? I think it's the same, except on all platforms, -fmsc-version and -fms-compatibility version are considered. Only with the MSVC toolchain do we go to the triple step, then the exe, then default to 18 with -fms-extensions. I guess this lets people compile code for Linux while pretending to be MSVC.

Right, so I think that there is actually *no* change from the previous behavior. The tests in Misc/diag-format.c in particular failed without this (MSVC2010 needs to be detected for the off-by-one in the diagnostics). The only code path that didn't appear to be tested was what I'm adding in https://reviews.llvm.org/D27498.

It might make sense for the MSVC logic to only live in the MSVC toolchain, and I'd be happy to move it there if you agree, but I'd prefer to do that as a follow-up, since this is already sort of non-trivial.

lib/Driver/MSVCToolChain.cpp

476 ↗

(On Diff #80526)

Ah, sorry about that... I misunderstood your previous comment.

[dlj]

Updated for rnk's comments.

[rnk]

lgtm

cfe/trunk/include/clang/Driver/ToolChain.h

public:
   /// \brief Add arguments to use system-specific CUDA includes.
   virtual void AddCudaIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                                   llvm::opt::ArgStringList &CC1Args) const;
 
   /// \brief Add arguments to use MCU GCC toolchain includes.
   virtual void AddIAMCUIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                                    llvm::opt::ArgStringList &CC1Args) const;
 
+  /// \brief On Windows, returns the MSVC compatibility version.
+  virtual VersionTuple computeMSVCVersion(const Driver *D,
+                                          const llvm::opt::ArgList &Args) const;
+
   /// \brief Return sanitizers which are available in this toolchain.
   virtual SanitizerMask getSupportedSanitizers() const;
 
   /// \brief Return sanitizers which are enabled by default.
   virtual SanitizerMask getDefaultSanitizers() const { return 0; }
-
-  /// \brief On Windows, returns the version of cl.exe.  On other platforms,
-  /// returns an empty VersionTuple.
-  virtual VersionTuple getMSVCVersionFromExe() const { return VersionTuple(); }
 };
 
 /// Set a ToolChain's effective triple. Reset it when the registration object
 /// is destroyed.
 class RegisterEffectiveTriple {
   const ToolChain &TC;
 
 public:

cfe/trunk/lib/Driver/MSVCToolChain.cpp

 #include "Tools.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/Version.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSwitch.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
bool MSVCToolChain::getVisualStudioBinariesFolder(const char *clangProgramPath,
   default:
     // Whatever this is, Visual Studio doesn't have a toolchain for it.
     return false;
   }
   path = BinDir.str();
   return true;
 }
 
+VersionTuple MSVCToolChain::getMSVCVersionFromTriple() const {
+  unsigned Major, Minor, Micro;
+  getTriple().getEnvironmentVersion(Major, Minor, Micro);
+  if (Major || Minor || Micro)
+    return VersionTuple(Major, Minor, Micro);
+  return VersionTuple();
+}
+
 VersionTuple MSVCToolChain::getMSVCVersionFromExe() const {
   VersionTuple Version;
 #ifdef USE_WIN32
   std::string BinPath;
   if (!getVisualStudioBinariesFolder("", BinPath))
     return Version;
   SmallString<128> ClExe(BinPath);
   llvm::sys::path::append(ClExe, "cl.exe");

 #endif
 }
 
 void MSVCToolChain::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
                                                  ArgStringList &CC1Args) const {
   // FIXME: There should probably be logic here to find libc++ on Windows.
 }
 
+VersionTuple MSVCToolChain::computeMSVCVersion(const Driver *D,
+                                               const ArgList &Args) const {
+  bool IsWindowsMSVC = getTriple().isWindowsMSVCEnvironment();
+  VersionTuple MSVT = ToolChain::computeMSVCVersion(D, Args);
+  if (MSVT.empty()) MSVT = getMSVCVersionFromTriple();
+  if (MSVT.empty() && IsWindowsMSVC) MSVT = getMSVCVersionFromExe();
+  if (MSVT.empty() &&
+      Args.hasFlag(options::OPT_fms_extensions, options::OPT_fno_ms_extensions,
+                   IsWindowsMSVC)) {
+    // -fms-compatibility-version=18.00 is default.
+    // FIXME: Consider bumping this to 19 (MSVC2015) soon.
+    MSVT = VersionTuple(18);
+  }
+  return MSVT;
+}
+
 std::string
 MSVCToolChain::ComputeEffectiveClangTriple(const ArgList &Args,
                                            types::ID InputType) const {
-  std::string TripleStr =
-      ToolChain::ComputeEffectiveClangTriple(Args, InputType);
-  llvm::Triple Triple(TripleStr);
-  VersionTuple MSVT =
-      tools::visualstudio::getMSVCVersion(/*D=*/nullptr, *this, Triple, Args,
-                                          /*IsWindowsMSVC=*/true);
-  if (MSVT.empty())
-    return TripleStr;
-
+  // The MSVC version doesn't care about the architecture, even though it
+  // may look at the triple internally.
+  VersionTuple MSVT = computeMSVCVersion(/*D=*/nullptr, Args);
   MSVT = VersionTuple(MSVT.getMajor(), MSVT.getMinor().getValueOr(0),
                       MSVT.getSubminor().getValueOr(0));
 
+  // For the rest of the triple, however, a computed architecture name may
+  // be needed.
+  llvm::Triple Triple(ToolChain::ComputeEffectiveClangTriple(Args, InputType));
   if (Triple.getEnvironment() == llvm::Triple::MSVC) {
     StringRef ObjFmt = Triple.getEnvironmentName().split('-').second;
     if (ObjFmt.empty())
       Triple.setEnvironmentName((Twine("msvc") + MSVT.getAsString()).str());
     else
       Triple.setEnvironmentName(
           (Twine("msvc") + MSVT.getAsString() + Twine('-') + ObjFmt).str());
   }

cfe/trunk/lib/Driver/ToolChain.cpp

SanitizerMask ToolChain::getSupportedSanitizers() const {
   return Res;
 }
 
 void ToolChain::AddCudaIncludeArgs(const ArgList &DriverArgs,
                                    ArgStringList &CC1Args) const {}
 
 void ToolChain::AddIAMCUIncludeArgs(const ArgList &DriverArgs,
                                     ArgStringList &CC1Args) const {}
+
+static VersionTuple separateMSVCFullVersion(unsigned Version) {
+  if (Version < 100)
+    return VersionTuple(Version);
+
+  if (Version < 10000)
+    return VersionTuple(Version / 100, Version % 100);
+
+  unsigned Build = 0, Factor = 1;
+  for (; Version > 10000; Version = Version / 10, Factor = Factor * 10)
+    Build = Build + (Version % 10) * Factor;
+  return VersionTuple(Version / 100, Version % 100, Build);
+}
+
+VersionTuple
+ToolChain::computeMSVCVersion(const Driver *D,
+                              const llvm::opt::ArgList &Args) const {
+  const Arg *MSCVersion = Args.getLastArg(options::OPT_fmsc_version);
+  const Arg *MSCompatibilityVersion =
+      Args.getLastArg(options::OPT_fms_compatibility_version);
+
+  if (MSCVersion && MSCompatibilityVersion) {
+    if (D)
+      D->Diag(diag::err_drv_argument_not_allowed_with)
+          << MSCVersion->getAsString(Args)
+          << MSCompatibilityVersion->getAsString(Args);
+    return VersionTuple();
+  }
+
+  if (MSCompatibilityVersion) {
+    VersionTuple MSVT;
+    if (MSVT.tryParse(MSCompatibilityVersion->getValue())) {
+      if (D)
+        D->Diag(diag::err_drv_invalid_value)
+            << MSCompatibilityVersion->getAsString(Args)
+            << MSCompatibilityVersion->getValue();
+    } else {
+      return MSVT;
+    }
+  }
+
+  if (MSCVersion) {
+    unsigned Version = 0;
+    if (StringRef(MSCVersion->getValue()).getAsInteger(10, Version)) {
+      if (D)
+        D->Diag(diag::err_drv_invalid_value)
+            << MSCVersion->getAsString(Args) << MSCVersion->getValue();
+    } else {
+      return separateMSVCFullVersion(Version);
+    }
+  }
+
+  return VersionTuple();
+}

cfe/trunk/lib/Driver/ToolChains.h

public:
   bool getWindowsSDKLibraryPath(std::string &path) const;
   /// \brief Check if Universal CRT should be used if available
   bool useUniversalCRT(std::string &visualStudioDir) const;
   bool getUniversalCRTSdkDir(std::string &path, std::string &ucrtVersion) const;
   bool getUniversalCRTLibraryPath(std::string &path) const;
   bool getVisualStudioInstallDir(std::string &path) const;
   bool getVisualStudioBinariesFolder(const char *clangProgramPath,
                                      std::string &path) const;
-  VersionTuple getMSVCVersionFromExe() const override;
+  VersionTuple
+  computeMSVCVersion(const Driver *D,
+                     const llvm::opt::ArgList &Args) const override;
 
   std::string ComputeEffectiveClangTriple(const llvm::opt::ArgList &Args,
                                           types::ID InputType) const override;
   SanitizerMask getSupportedSanitizers() const override;
 
 protected:
   void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList &DriverArgs,
                                      llvm::opt::ArgStringList &CC1Args,
                                      const std::string &folder,
                                      const Twine &subfolder1,
                                      const Twine &subfolder2 = "",
                                      const Twine &subfolder3 = "") const;
 
   Tool *buildLinker() const override;
   Tool *buildAssembler() const override;
+private:
+  VersionTuple getMSVCVersionFromTriple() const;
+  VersionTuple getMSVCVersionFromExe() const;
 };
 
 class LLVM_LIBRARY_VISIBILITY CrossWindowsToolChain : public Generic_GCC {
 public:
   CrossWindowsToolChain(const Driver &D, const llvm::Triple &T,
                         const llvm::opt::ArgList &Args);
 
   bool IsIntegratedAssemblerDefault() const override { return true; }

cfe/trunk/lib/Driver/Tools.h

void ConstructJob(Compilation &C, const JobAction &JA,
                     const InputInfo &Output, const InputInfoList &Inputs,
                     const llvm::opt::ArgList &TCArgs,
                     const char *LinkingOutput) const override;
 };
 } // end namespace dragonfly
 
 /// Visual studio tools.
 namespace visualstudio {
-VersionTuple getMSVCVersion(const Driver *D, const ToolChain &TC,
-                            const llvm::Triple &Triple,
-                            const llvm::opt::ArgList &Args, bool IsWindowsMSVC);
-
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
 public:
   Linker(const ToolChain &TC)
       : Tool("visualstudio::Linker", "linker", TC, RF_Full,
              llvm::sys::WEM_UTF16) {}
 
   bool hasIntegratedCPP() const override { return false; }
   bool isLinkJob() const override { return true; }

cfe/trunk/lib/Driver/Tools.cpp

static void addDashXForInput(const ArgList &Args, const InputInfo &Input,
 
   CmdArgs.push_back("-x");
   if (Args.hasArg(options::OPT_rewrite_objc))
     CmdArgs.push_back(types::getTypeName(types::TY_PP_ObjCXX));
   else
     CmdArgs.push_back(types::getTypeName(Input.getType()));
 }
 
-static VersionTuple getMSCompatibilityVersion(unsigned Version) {
-  if (Version < 100)
-    return VersionTuple(Version);
-
-  if (Version < 10000)
-    return VersionTuple(Version / 100, Version % 100);
-
-  unsigned Build = 0, Factor = 1;
-  for (; Version > 10000; Version = Version / 10, Factor = Factor * 10)
-    Build = Build + (Version % 10) * Factor;
-  return VersionTuple(Version / 100, Version % 100, Build);
-}
-
 // Claim options we don't want to warn if they are unused. We do this for
 // options that build systems might add but are unused when assembling or only
 // running the preprocessor for example.
 static void claimNoWarnArgs(const ArgList &Args) {
   // Don't warn about unused -f(no-)?lto.  This can happen when we're
   // preprocessing, precompiling or assembling.
   Args.ClaimAllArgs(options::OPT_flto_EQ);
   Args.ClaimAllArgs(options::OPT_flto);
#ifdef LLVM_ON_UNIX
   std::string UID = llvm::utostr(getuid());
 #else
   // FIXME: Windows seems to have an 'SID' that might work.
   std::string UID = "9999";
 #endif
   Result.append(UID.begin(), UID.end());
 }
 
-VersionTuple visualstudio::getMSVCVersion(const Driver *D, const ToolChain &TC,
-                                          const llvm::Triple &Triple,
-                                          const llvm::opt::ArgList &Args,
-                                          bool IsWindowsMSVC) {
-  if (Args.hasFlag(options::OPT_fms_extensions, options::OPT_fno_ms_extensions,
-                   IsWindowsMSVC) ||
-      Args.hasArg(options::OPT_fmsc_version) ||
-      Args.hasArg(options::OPT_fms_compatibility_version)) {
-    const Arg *MSCVersion = Args.getLastArg(options::OPT_fmsc_version);
-    const Arg *MSCompatibilityVersion =
-        Args.getLastArg(options::OPT_fms_compatibility_version);
-
-    if (MSCVersion && MSCompatibilityVersion) {
-      if (D)
-        D->Diag(diag::err_drv_argument_not_allowed_with)
-            << MSCVersion->getAsString(Args)
-            << MSCompatibilityVersion->getAsString(Args);
-      return VersionTuple();
-    }
-
-    if (MSCompatibilityVersion) {
-      VersionTuple MSVT;
-      if (MSVT.tryParse(MSCompatibilityVersion->getValue()) && D)
-        D->Diag(diag::err_drv_invalid_value)
-            << MSCompatibilityVersion->getAsString(Args)
-            << MSCompatibilityVersion->getValue();
-      return MSVT;
-    }
-
-    if (MSCVersion) {
-      unsigned Version = 0;
-      if (StringRef(MSCVersion->getValue()).getAsInteger(10, Version) && D)
-        D->Diag(diag::err_drv_invalid_value) << MSCVersion->getAsString(Args)
-                                             << MSCVersion->getValue();
-      return getMSCompatibilityVersion(Version);
-    }
-
-    unsigned Major, Minor, Micro;
-    Triple.getEnvironmentVersion(Major, Minor, Micro);
-    if (Major || Minor || Micro)
-      return VersionTuple(Major, Minor, Micro);
-
-    if (IsWindowsMSVC) {
-      VersionTuple MSVT = TC.getMSVCVersionFromExe();
-      if (!MSVT.empty())
-        return MSVT;
-
-      // FIXME: Consider bumping this to 19 (MSVC2015) soon.
-      return VersionTuple(18);
-    }
-  }
-  return VersionTuple();
-}
-
 static Arg *getLastProfileUseArg(const ArgList &Args) {
   auto *ProfileUseArg = Args.getLastArg(
       options::OPT_fprofile_instr_use, options::OPT_fprofile_instr_use_EQ,
       options::OPT_fprofile_use, options::OPT_fprofile_use_EQ,
       options::OPT_fno_profile_instr_use);
 
   if (ProfileUseArg &&
       ProfileUseArg->getOption().matches(options::OPT_fno_profile_instr_use))

   // -fms-compatibility=0 is default.
   if (Args.hasFlag(options::OPT_fms_compatibility,
                    options::OPT_fno_ms_compatibility,
                    (IsWindowsMSVC &&
                     Args.hasFlag(options::OPT_fms_extensions,
                                  options::OPT_fno_ms_extensions, true))))
     CmdArgs.push_back("-fms-compatibility");
 
-  // -fms-compatibility-version=18.00 is default.
-  VersionTuple MSVT = visualstudio::getMSVCVersion(
-      &D, getToolChain(), getToolChain().getTriple(), Args, IsWindowsMSVC);
+  VersionTuple MSVT =
+      getToolChain().computeMSVCVersion(&getToolChain().getDriver(), Args);
   if (!MSVT.empty())
     CmdArgs.push_back(
         Args.MakeArgString("-fms-compatibility-version=" + MSVT.getAsString()));
 
   bool IsMSVC2015Compatible = MSVT.getMajor() >= 19;
   if (ImplyVCPPCXXVer) {
     StringRef LanguageStandard;
     if (const Arg *StdArg = Args.getLastArg(options::OPT__SLASH_std)) {