On Wed, May 11, 2016 at 8:29 AM, Nico Weber via cfe-commits <cfe-commits@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@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
