[PATCH] D20136: Get default -fms-compatibility-version from cl.exe's version

[David Majnemer via cfe-commits]

majnemer added inline comments.

================
Comment at: lib/Driver/MSVCToolChain.cpp:472
@@ +471,3 @@
+
+  const DWORD VersionSize = ::GetFileVersionInfoSizeA(ClExe.c_str(), nullptr);
+  if (VersionSize == 0) {
----------------
amccarth wrote:
> majnemer wrote:
> > Why not use the `GetFileVersionInfoSizeW` variant?
> I started down that road, but it seemed overkill to convert the path to a wide string.  I'm happy to do it if you think it worthwhile.
I think it's worth it, we get bug reports whenever we break this sort of thing...

================
Comment at: lib/Driver/MSVCToolChain.cpp:477
@@ +476,3 @@
+  std::vector<char> VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoA(ClExe.c_str(), 0, VersionSize,
+                             VersionBlock.data())) {
----------------
amccarth wrote:
> thakis wrote:
> > We already stat a bunch of directories to find the sdk include path. Can we use the result of that instead of looking at cl.exe? Then we wouldn't have to do additional stats.
> I'm just a simple CPM/VMS/Windows developer.  Your Linux terms (stat) frighten and confuse me.
> 
> Seriously, though, this API isn't a file system check.  It's digging out the version record from the file's resources.
> 
> We _could_ guess at the version from the names of the directories in the path, but that would require mapping names to versions, and if it's installed in a non-standard place it wouldn't help at all.
> 
> Also, `-fms-compatibility-version` is really about the version of the compiler (cl.exe), not that of the standard library nor of the SDK, so trying to check something else as a proxy for the version seems prone to obscure failures.
> 
> I share your concern about speed, especially since getting the version happens twice (once for the triple and once for the compatibility version), but invoking clang and having it choose the wrong default costs a lot of time, too.
> 
> The bug report correctly says we shouldn't spin up a process to run `cl /version`--that would be far more expensive.  And if you put `-fms-compatibility-version` on the command line, then this function won't be called as it won't need to figure out the default.
> Seriously, though, this API isn't a file system check. It's digging out the version record from the file's resources.

Isn't the content stored as a resource in the PE?  If so, that means that getting the version information requires reading bytes inside of cl.exe

With regard to `-fms-compatibility-version`, it shouldn't have anything to do with the platform SDK.  However, it is fundamentally the case that the CRT and the compiler have the same version.  Otherwise, really terrible things happen (MSVC19 assumes char16_t is a keyword which it provides but the MSVC18 STL assumes it is responsible for providing the keyword).


http://reviews.llvm.org/D20136