amccarth added inline comments. ================ Comment at: lib/Driver/MSVCToolChain.cpp:478 @@ +477,3 @@ + + const DWORD VersionSize = ::GetFileVersionInfoSizeW(ClExeWide.c_str(), + nullptr); ---------------- amccarth wrote: > 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. > > 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'll do some research to figure out the actual costs. I suspect that walking the PATH for the executable may be far more expensive, but I'll get some numbers and report back.
http://reviews.llvm.org/D20136 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
