On Wed, May 11, 2016 at 11:00 AM, Adrian McCarthy via cfe-commits <cfe-commits@lists.llvm.org> wrote: > amccarth added inline comments. > > ================ > Comment at: lib/Driver/MSVCToolChain.cpp:478 > @@ +477,3 @@ > + > + const DWORD VersionSize = ::GetFileVersionInfoSizeW(ClExeWide.c_str(), > + nullptr); > ---------------- > thakis wrote: >> 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. > The version number in the directory name and the registry is the version > number of Visual Studio not of the compiler. Yes, we could do a mapping (VS > 14 comes bundled with CL 19), assuming Microsoft continues to keep VS > releases and compiler releases in sync, and it means this code will forever > need updates to the mapping data. > > The mapping would give just the major version number, which might be all that > matters now, but if there's ever a CL 19.1 that has different compatibility > requirements (and is maybe released out-of-band with Visual Studio), we'd be > stuck.
The Updates to MSVC will change the version number (but not the major version), for instance. > Getting the actual version from the compiler seems the most accurate and > future-proof way to check. If that's too expensive, then maybe we should > abandon the idea of detecting the default for compatibility. I think that abandoning the idea would be a shame. The discussion about perf is a good one to have, but I don't think it's sufficient to abandon the idea unless we have some actual measurements to provide concrete data demonstrating that the perf hit is unacceptable. ~Aaron _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
