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

[Aaron Ballman via cfe-commits]

On Wed, May 11, 2016 at 8:29 AM, Nico Weber via cfe-commits
<cfe-commits at lists.llvm.org> wrote:
> thakis added inline comments.
>
> ================
> Comment at: lib/Driver/MSVCToolChain.cpp:478
> @@ +477,3 @@
> +
> +  const DWORD VersionSize = ::GetFileVersionInfoSizeW(ClExeWide.c_str(),
> +                                                      nullptr);
> ----------------
> amccarth wrote:
>> Yes, it looks in the executable (which I tried to emphasize with the method name).
>>
>> I don't think this is very expensive given that Explorer often makes zillions of such calls, but I'm open to other suggestions.
>>
>> I know that you can't use a library that's newer than the compiler (because it may use new language features), but I don't know if that applies in the other direction or how we would safely and reliably map directory names to library versions and therefore to compiler versions.
> I agree that figuring out the right value for fmsc-version automatically somehow is definitely something we should do.
>
> I forgot that `getVisualStudioBinariesFolder` already works by looking for cl.exe in PATH, so cl.exe's metadata is already warmed up in the disk cache. However, GetFileVersionInfoW() probably opens cl.exe itself and does some PE parsing to get at the version, and that probably is in cold cache territory. (https://msdn.microsoft.com/en-us/library/windows/desktop/ms647003(v=vs.85).aspx suggests that this function might open several files).
>
> `getVisualStudioBinariesFolder` checks:
>
> 1. getenv("VCINSTALLDIR")
> 2. cl.exe in getenv("PATH")
> 3. registry (via getVisualStudioInstallDir)
>
> The common cases are 1 and 3. For 1, for default installs, the version number is part of the directory name (for default installs, what most people have). For 3, the version number is in the registry key we query. So in most cases we shouldn't have to look at cl.exe itself. And for the cases where we would have to look, maybe it's ok to require an explicit fmsc-version flag.

I agree that in most cases we shouldn't have to look at cl.exe itself,
but I think it's better for the users in the other case that we just
open cl.exe instead of force them to specify a flag that we could just
query ourselves. Yes, it's a performance hit (though I don't know that
I've seen any measurements to suggest it's a bad perf hit in
practice). However, it's also a usability gain and would be consistent
behavior with default installs.

~Aaron

>
>
> http://reviews.llvm.org/D20136
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits