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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In https://reviews.llvm.org/D20136#640689, @amccarth wrote:

> > and folks have to manually add mincore.lib to their link.
>
> I could load the library dynamically on demand and use GetProcAddress, but I 
> suspect that would further degrade the performance.  I could probably write 
> my own code to find the version in the binary, but I doubt that crosses the 
> reward/work threshold.


`#pragma comment(lib, "mincore")`, maybe?


Repository:
  rL LLVM

https://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

> and folks have to manually add mincore.lib to their link.

I could load the library dynamically on demand and use GetProcAddress, but I 
suspect that would further degrade the performance.  I could probably write my 
own code to find the version in the binary, but I doubt that crosses the 
reward/work threshold.


Repository:
  rL LLVM

https://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

One consequence from this that I just realized is that linking a binary 
depending on clang stuff with (morally):

  c++ -o foo foo.o $($LLVMBUILD/bin/llvm-config --ldflags) -lclangFrontend 
-lclangDriver -lclangParse -lclangSema -lclangSerialization -lclangAnalysis 
-lclangAST -lclangEdit -lclangLex -lclangBasic $($LLVMBUILD/bin/llvm-config 
--libs) $($LLVMBUILD/bin/llvm-config --system-libs)

now fails with

  clangDriver.lib(MSVCToolChain.cpp.obj) : error LNK2019: unresolved external 
symbol GetFileVersionInfoSizeW referenced in
   function "private: class clang::VersionTuple __cdecl 
clang::driver::toolchains::MSVCToolChain::getMSVCVersionFromExe(vo
  id)const " 
(?getMSVCVersionFromExe@MSVCToolChain@toolchains@driver@clang@@AEBA?AVVersionTuple@4@XZ)
  clangDriver.lib(MSVCToolChain.cpp.obj) : error LNK2019: unresolved external 
symbol GetFileVersionInfoW referenced in fun
  ction "private: class clang::VersionTuple __cdecl 
clang::driver::toolchains::MSVCToolChain::getMSVCVersionFromExe(void)c
  onst " 
(?getMSVCVersionFromExe@MSVCToolChain@toolchains@driver@clang@@AEBA?AVVersionTuple@4@XZ)
  clangDriver.lib(MSVCToolChain.cpp.obj) : error LNK2019: unresolved external 
symbol VerQueryValueW referenced in function
   "private: class clang::VersionTuple __cdecl 
clang::driver::toolchains::MSVCToolChain::getMSVCVersionFromExe(void)const
  " 
(?getMSVCVersionFromExe@MSVCToolChain@toolchains@driver@clang@@AEBA?AVVersionTuple@4@XZ)
  pptest.exe : fatal error LNK1120: 3 unresolved externals

and folks have to manually add mincore.lib to their link. `llvm-config 
--system-libs` takes care of things like this for LLVM, but we don't have a 
`clang-config` as far as I know (does anybody know why not?). So folks now have 
to manually pass more libraries than just the clang libraries. Not a big deal, 
but I figured I'd point it out.


Repository:
  rL LLVM

https://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

This revision was automatically updated to reflect the committed changes.
amccarth marked an inline comment as done.
Closed by commit rL269515: Get default -fms-compatibility-version from cl.exe's 
version (authored by amccarth).

Changed prior to commit:
  http://reviews.llvm.org/D20136?vs=57196=57267#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20136

Files:
  cfe/trunk/include/clang/Driver/ToolChain.h
  cfe/trunk/lib/Driver/MSVCToolChain.cpp
  cfe/trunk/lib/Driver/ToolChains.h
  cfe/trunk/lib/Driver/Tools.cpp
  cfe/trunk/lib/Driver/Tools.h
  cfe/trunk/test/Driver/cl-options.c
  cfe/trunk/test/Driver/msc-version.c
  cfe/trunk/test/Driver/msvc-triple.c
  cfe/trunk/test/Misc/diag-format.c

Index: cfe/trunk/include/clang/Driver/ToolChain.h
===
--- cfe/trunk/include/clang/Driver/ToolChain.h
+++ cfe/trunk/include/clang/Driver/ToolChain.h
@@ -11,6 +11,7 @@
 #define LLVM_CLANG_DRIVER_TOOLCHAIN_H
 
 #include "clang/Basic/Sanitizers.h"
+#include "clang/Basic/VersionTuple.h"
 #include "clang/Driver/Action.h"
 #include "clang/Driver/Multilib.h"
 #include "clang/Driver/Types.h"
@@ -422,6 +423,10 @@
 
   /// \brief Return sanitizers which are enabled by default.
   virtual SanitizerMask getDefaultSanitizers() const { return 0; }
+
+  /// \brief On Windows, returns the version of cl.exe.  On other platforms,
+  /// returns an empty VersionTuple.
+  virtual VersionTuple getMSVCVersionFromExe() const { return VersionTuple(); }
 };
 
 } // end namespace driver
Index: cfe/trunk/test/Misc/diag-format.c
===
--- cfe/trunk/test/Misc/diag-format.c
+++ cfe/trunk/test/Misc/diag-format.c
@@ -37,7 +37,7 @@
 // DEFAULT: {{.*}}:36:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2010: {{.*}}(36,7) : warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2013: {{.*}}(36,8) : warning: extra tokens at end of #endif directive [-Wextra-tokens]
-// MSVC: {{.*}}(36,8) : warning: extra tokens at end of #endif directive [-Wextra-tokens]
+// MSVC: {{.*}}(36,8){{ ?}}: warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2015: {{.*}}(36,8): warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // VI: {{.*}} +36:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2015_ORIG: {{.*}}(36): warning: extra tokens at end of #endif directive [-Wextra-tokens]
Index: cfe/trunk/test/Driver/msvc-triple.c
===
--- cfe/trunk/test/Driver/msvc-triple.c
+++ cfe/trunk/test/Driver/msvc-triple.c
@@ -1,9 +1,7 @@
-// RUN: %clang -target i686-pc-windows-msvc   -S -emit-llvm %s -o - | FileCheck %s --check-prefix=DEFAULT
 // RUN: %clang -target i686-pc-windows-msvc19 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=TARGET-19
 // RUN: %clang -target i686-pc-windows-msvc   -S -emit-llvm %s -o - -fms-compatibility-version=19 | FileCheck %s --check-prefix=OVERRIDE-19
 // RUN: %clang -target i686-pc-windows-msvc-elf -S -emit-llvm %s -o - | FileCheck %s --check-prefix=ELF-DEFAULT
 
-// DEFAULT: target triple = "i686-pc-windows-msvc18.0.0"
 // TARGET-19:   target triple = "i686-pc-windows-msvc19.0.0"
 // OVERRIDE-19: target triple = "i686-pc-windows-msvc19.0.0"
-// ELF-DEFAULT: target triple = "i686-pc-windows-msvc18.0.0-elf"
+// ELF-DEFAULT: target triple = "i686-pc-windows-msvc{{.*}}-elf"
Index: cfe/trunk/test/Driver/cl-options.c
===
--- cfe/trunk/test/Driver/cl-options.c
+++ cfe/trunk/test/Driver/cl-options.c
@@ -387,7 +387,7 @@
 // RTTI-NOT: "-fno-rtti"
 
 // thread safe statics are off for versions < 19.
-// RUN: %clang_cl /c -### -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s
+// RUN: %clang_cl /c -### -fms-compatibility-version=18 -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s
 // RUN: %clang_cl /Zc:threadSafeInit /Zc:threadSafeInit- /c -### -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s
 // NoThreadSafeStatics: "-fno-threadsafe-statics"
 
Index: cfe/trunk/test/Driver/msc-version.c
===
--- cfe/trunk/test/Driver/msc-version.c
+++ cfe/trunk/test/Driver/msc-version.c
@@ -1,15 +1,4 @@
 //
-// Verify defaults
-//
-
-// RUN: %clang -target i686-windows -fms-compatibility -dM -E - 
+
+  #pragma comment(lib, "version.lib")
 #endif
 
 using namespace clang::driver;
@@ -457,6 +460,45 @@
   return true;
 }
 
+VersionTuple MSVCToolChain::getMSVCVersionFromExe() const {
+  VersionTuple Version;
+#ifdef USE_WIN32
+  std::string BinPath;
+  if (!getVisualStudioBinariesFolder("", BinPath))
+return Version;
+  SmallString<128> ClExe = BinPath;
+  llvm::sys::path::append(ClExe, "cl.exe");
+
+  std::wstring ClExeWide;
+  if (!llvm::ConvertUTF8toWide(ClExe.c_str(), ClExeWide))
+return Version;
+
+  const DWORD 

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

[Reid Kleckner via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

LG, sounds like people are happy with this


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

amccarth marked an inline comment as done.


Comment at: lib/Driver/MSVCToolChain.cpp:481
@@ +480,3 @@
+
+  std::vector VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoW(ClExeWide.c_str(), 0, VersionSize,

majnemer wrote:
> amccarth wrote:
> > majnemer wrote:
> > > It might be nicer to use a `SmallVector > > sizeof(VS_FIXEDFILEINFO)>`, or whatever `VersionSize` typically is, here 
> > > to avoid heap allocation in the common case.
> > What's the cutoff for "small"?  The version block in cl.exe is about 9KB.
> Using 10K is probably fine, the default stack size on Windows is a massive 1 
> MB and this function is not reentrant.
My mistake.  It's a smidge over 1KB, (still more than sizeof(VS_FIXEDFILEINFO)) 
so I've make a SmallVector of 2KB.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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:481
@@ +480,3 @@
+
+  std::vector VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoW(ClExeWide.c_str(), 0, VersionSize,

amccarth wrote:
> majnemer wrote:
> > It might be nicer to use a `SmallVector > sizeof(VS_FIXEDFILEINFO)>`, or whatever `VersionSize` typically is, here to 
> > avoid heap allocation in the common case.
> What's the cutoff for "small"?  The version block in cl.exe is about 9KB.
Using 10K is probably fine, the default stack size on Windows is a massive 1 MB 
and this function is not reentrant.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

amccarth added inline comments.


Comment at: lib/Driver/MSVCToolChain.cpp:481
@@ +480,3 @@
+
+  std::vector VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoW(ClExeWide.c_str(), 0, VersionSize,

majnemer wrote:
> It might be nicer to use a `SmallVector`, 
> or whatever `VersionSize` typically is, here to avoid heap allocation in the 
> common case.
What's the cutoff for "small"?  The version block in cl.exe is about 9KB.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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:481
@@ +480,3 @@
+
+  std::vector VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoW(ClExeWide.c_str(), 0, VersionSize,

It might be nicer to use a `SmallVector`, or 
whatever `VersionSize` typically is, here to avoid heap allocation in the 
common case.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

amccarth updated this revision to Diff 57196.
amccarth marked an inline comment as done.
amccarth added a comment.

Addressed additional comments.


http://reviews.llvm.org/D20136

Files:
  include/clang/Driver/ToolChain.h
  lib/Driver/MSVCToolChain.cpp
  lib/Driver/ToolChains.h
  lib/Driver/Tools.cpp
  lib/Driver/Tools.h
  test/Driver/cl-options.c
  test/Driver/msc-version.c
  test/Driver/msvc-triple.c
  test/Misc/diag-format.c

Index: test/Misc/diag-format.c
===
--- test/Misc/diag-format.c
+++ test/Misc/diag-format.c
@@ -37,7 +37,7 @@
 // DEFAULT: {{.*}}:36:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2010: {{.*}}(36,7) : warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2013: {{.*}}(36,8) : warning: extra tokens at end of #endif directive [-Wextra-tokens]
-// MSVC: {{.*}}(36,8) : warning: extra tokens at end of #endif directive [-Wextra-tokens]
+// MSVC: {{.*}}(36,8){{ ?}}: warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2015: {{.*}}(36,8): warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // VI: {{.*}} +36:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2015_ORIG: {{.*}}(36): warning: extra tokens at end of #endif directive [-Wextra-tokens]
Index: test/Driver/msvc-triple.c
===
--- test/Driver/msvc-triple.c
+++ test/Driver/msvc-triple.c
@@ -1,9 +1,7 @@
-// RUN: %clang -target i686-pc-windows-msvc   -S -emit-llvm %s -o - | FileCheck %s --check-prefix=DEFAULT
 // RUN: %clang -target i686-pc-windows-msvc19 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=TARGET-19
 // RUN: %clang -target i686-pc-windows-msvc   -S -emit-llvm %s -o - -fms-compatibility-version=19 | FileCheck %s --check-prefix=OVERRIDE-19
 // RUN: %clang -target i686-pc-windows-msvc-elf -S -emit-llvm %s -o - | FileCheck %s --check-prefix=ELF-DEFAULT
 
-// DEFAULT: target triple = "i686-pc-windows-msvc18.0.0"
 // TARGET-19:   target triple = "i686-pc-windows-msvc19.0.0"
 // OVERRIDE-19: target triple = "i686-pc-windows-msvc19.0.0"
-// ELF-DEFAULT: target triple = "i686-pc-windows-msvc18.0.0-elf"
+// ELF-DEFAULT: target triple = "i686-pc-windows-msvc{{.*}}-elf"
Index: test/Driver/msc-version.c
===
--- test/Driver/msc-version.c
+++ test/Driver/msc-version.c
@@ -1,15 +1,4 @@
 //
-// Verify defaults
-//
-
-// RUN: %clang -target i686-windows -fms-compatibility -dM -E - &1 | FileCheck -check-prefix=NoThreadSafeStatics %s
+// RUN: %clang_cl /c -### -fms-compatibility-version=18 -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s
 // RUN: %clang_cl /Zc:threadSafeInit /Zc:threadSafeInit- /c -### -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s
 // NoThreadSafeStatics: "-fno-threadsafe-statics"
 
Index: lib/Driver/Tools.h
===
--- lib/Driver/Tools.h
+++ lib/Driver/Tools.h
@@ -682,7 +682,8 @@
 
 /// Visual studio tools.
 namespace visualstudio {
-VersionTuple getMSVCVersion(const Driver *D, const llvm::Triple ,
+VersionTuple getMSVCVersion(const Driver *D, const ToolChain ,
+const llvm::Triple ,
 const llvm::opt::ArgList , bool IsWindowsMSVC);
 
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -3291,7 +3291,7 @@
   Result.append(UID.begin(), UID.end());
 }
 
-VersionTuple visualstudio::getMSVCVersion(const Driver *D,
+VersionTuple visualstudio::getMSVCVersion(const Driver *D, const ToolChain ,
   const llvm::Triple ,
   const llvm::opt::ArgList ,
   bool IsWindowsMSVC) {
@@ -,8 +,14 @@
 if (Major || Minor || Micro)
   return VersionTuple(Major, Minor, Micro);
 
-// FIXME: Consider bumping this to 19 (MSVC2015) soon.
-return VersionTuple(18);
+if (IsWindowsMSVC) {
+  VersionTuple MSVT = TC.getMSVCVersionFromExe();
+  if (!MSVT.empty())
+return MSVT;
+
+  // FIXME: Consider bumping this to 19 (MSVC2015) soon.
+  return VersionTuple(18);
+}
   }
   return VersionTuple();
 }
@@ -5224,7 +5230,7 @@
 
   // -fms-compatibility-version=18.00 is default.
   VersionTuple MSVT = visualstudio::getMSVCVersion(
-  , getToolChain().getTriple(), Args, IsWindowsMSVC);
+  , getToolChain(), getToolChain().getTriple(), Args, IsWindowsMSVC);
   if (!MSVT.empty())
 CmdArgs.push_back(
 Args.MakeArgString("-fms-compatibility-version=" + MSVT.getAsString()));
Index: lib/Driver/ToolChains.h
===
--- 

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

[Adrian McCarthy via cfe-commits]

amccarth marked 2 inline comments as done.


Comment at: lib/Driver/MSVCToolChain.cpp:42
@@ -40,1 +41,3 @@
+
+  #pragma comment(lib, "version.lib")
 #endif

aaron.ballman wrote:
> Eh, I am lightening up on this sort of thing, so this is fine by me.
I was following the pattern I saw in llvm\lib\Support\Windows\Path.inc (and 
elsewhere), so I thought it was the way we did things around here.


Comment at: lib/Driver/MSVCToolChain.cpp:483
@@ +482,3 @@
+  }
+  std::vector VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoW(ClExeWide.c_str(), 0, VersionSize,

aaron.ballman wrote:
> Pure pedantry: `uint8_t` instead of `char`, or is this data really a textual 
> string in practice?
It's a mix.  The part we're looking at is binary data, but the rest of the 
block is text.

I though the API wanted a pointer to char, so I chose `char` to avoid 
unnecessary casts.

But I must've misread the reference page, because just now I double-checked and 
I see that the API wants a void pointer, so I'll go ahead and use `uint8_t`, 
which satisfies my inner pedant as well.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.


Comment at: lib/Driver/MSVCToolChain.cpp:42
@@ -40,1 +41,3 @@
+
+  #pragma comment(lib, "version.lib")
 #endif

Eh, I am lightening up on this sort of thing, so this is fine by me.


Comment at: lib/Driver/MSVCToolChain.cpp:467
@@ +466,3 @@
+  std::string BinPath;
+  if (!getVisualStudioBinariesFolder("", BinPath)) {
+return Version;

Elide braces (here and elsewhere) per usual project coding conventions.


Comment at: lib/Driver/MSVCToolChain.cpp:483
@@ +482,3 @@
+  }
+  std::vector VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoW(ClExeWide.c_str(), 0, VersionSize,

Pure pedantry: `uint8_t` instead of `char`, or is this data really a textual 
string in practice?


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

amccarth added a comment.

Are there any remaining concerns with this patch?  Is everyone satisfied with 
the performance numbers?


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via cfe-commits]

thakis added a comment.

Ok, that seems fine then. Thanks much for checking!


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

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:
> > > 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.
> > Compilers being released independently of VC versions and fractional compat 
> > numbers sounds like things we can worry about when they happen (probably 
> > not soon, right?).
> > 
> > We already walk PATH, so that wouldn't be an additional cost.
> > 
> > Be sure to measure cold disk cache perf impact (which is tricky on Windows 
> > since there's as far as I know no way to tell the OS to drop its caches). 
> > As far as I know file metadata is stored with the directory node on NTFS, 
> > so stating files doesn't warm up file content accesses.
> > Compilers being released independently of VC versions and fractional compat 
> > numbers sounds like things we can worry about when they happen (probably 
> > not soon, right?).
> 
> It already happens.  Herb Sutter talks about it in one of his blogs:  "Soon 
> after VC++11 ships we have announced we will do out-of-band releases for 
> additional C++11 conformance which will naturally also include more C11 
> features that are in the C subset of C++11."  In this case, it's just the 
> build number (of major.minor.build) that's updating, but it's for increasing 
> conformance, which is exactly a compatibility issue.
> 
> > We already walk PATH, so that wouldn't be an additional cost.
> 
> I suspect we may be walking it more than once, which can be expensive even if 
> the cache is all warmed up.  This is one of the things I'm checking.  If it's 
> a problem, I'll propose a patch to cache the result from the first walk.
> 
> > stating files doesn't warm up file content accesses.
> 
> That 

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

[Adrian McCarthy via cfe-commits]

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:
> > 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.
> Compilers being released independently of VC versions and fractional compat 
> numbers sounds like things we can worry about when they happen (probably not 
> soon, right?).
> 
> We already walk PATH, so that wouldn't be an additional cost.
> 
> Be sure to measure cold disk cache perf impact (which is tricky on Windows 
> since there's as far as I know no way to tell the OS to drop its caches). As 
> far as I know file metadata is stored with the directory node on NTFS, so 
> stating files doesn't warm up file content accesses.
> Compilers being released independently of VC versions and fractional compat 
> numbers sounds like things we can worry about when they happen (probably not 
> soon, right?).

It already happens.  Herb Sutter talks about it in one of his blogs:  "Soon 
after VC++11 ships we have announced we will do out-of-band releases for 
additional C++11 conformance which will naturally also include more C11 
features that are in the C subset of C++11."  In this case, it's just the build 
number (of major.minor.build) that's updating, but it's for increasing 
conformance, which is exactly a compatibility issue.

> We already walk PATH, so that wouldn't be an additional cost.

I suspect we may be walking it more than once, which can be expensive even if 
the cache is all warmed up.  This is one of the things I'm checking.  If it's a 
problem, I'll propose a patch to cache the result from the first walk.

> stating files doesn't warm up file content accesses.

That is correct.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

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

[Nico Weber via cfe-commits]

thakis added inline comments.


Comment at: lib/Driver/MSVCToolChain.cpp:478
@@ +477,3 @@
+
+  const DWORD VersionSize = ::GetFileVersionInfoSizeW(ClExeWide.c_str(),
+  nullptr);

amccarth wrote:
> 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.
Compilers being released independently of VC versions and fractional compat 
numbers sounds like things we can worry about when they happen (probably not 
soon, right?).

We already walk PATH, so that wouldn't be an additional cost.

Be sure to measure cold disk cache perf impact (which is tricky on Windows 
since there's as far as I know no way to tell the OS to drop its caches). As 
far as I know file metadata is stored with the directory node on NTFS, so 
stating files doesn't warm up file content accesses.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

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

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

[Aaron Ballman via cfe-commits]

On Wed, May 11, 2016 at 11:00 AM, Adrian McCarthy via cfe-commits
 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

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

[Adrian McCarthy via cfe-commits]

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.

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.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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
 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

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

[Nico Weber via cfe-commits]

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.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Robinson via cfe-commits]

probinson added a subscriber: probinson.
probinson added a comment.

In http://reviews.llvm.org/D20136#426586, @amccarth wrote:

> now using wide-chars for WinAPI calls,


Dōmo arigatō gozaimashita!


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

amccarth marked 4 inline comments as done.


Comment at: lib/Driver/MSVCToolChain.cpp:478
@@ +477,3 @@
+
+  const DWORD VersionSize = ::GetFileVersionInfoSizeW(ClExeWide.c_str(),
+  nullptr);

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.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

amccarth updated this revision to Diff 56833.
amccarth marked an inline comment as done.
amccarth added a comment.

Addressed most comments: now using wide-chars for WinAPI calls, made 
getMSVCVersionFromExe a virtual method, removed extraneous typo-correction.


http://reviews.llvm.org/D20136

Files:
  include/clang/Driver/ToolChain.h
  lib/Driver/MSVCToolChain.cpp
  lib/Driver/ToolChains.h
  lib/Driver/Tools.cpp
  lib/Driver/Tools.h
  test/Driver/cl-options.c
  test/Driver/msc-version.c
  test/Driver/msvc-triple.c
  test/Misc/diag-format.c

Index: test/Misc/diag-format.c
===
--- test/Misc/diag-format.c
+++ test/Misc/diag-format.c
@@ -37,7 +37,7 @@
 // DEFAULT: {{.*}}:36:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2010: {{.*}}(36,7) : warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2013: {{.*}}(36,8) : warning: extra tokens at end of #endif directive [-Wextra-tokens]
-// MSVC: {{.*}}(36,8) : warning: extra tokens at end of #endif directive [-Wextra-tokens]
+// MSVC: {{.*}}(36,8){{ ?}}: warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2015: {{.*}}(36,8): warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // VI: {{.*}} +36:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]
 // MSVC2015_ORIG: {{.*}}(36): warning: extra tokens at end of #endif directive [-Wextra-tokens]
Index: test/Driver/msvc-triple.c
===
--- test/Driver/msvc-triple.c
+++ test/Driver/msvc-triple.c
@@ -1,9 +1,7 @@
-// RUN: %clang -target i686-pc-windows-msvc   -S -emit-llvm %s -o - | FileCheck %s --check-prefix=DEFAULT
 // RUN: %clang -target i686-pc-windows-msvc19 -S -emit-llvm %s -o - | FileCheck %s --check-prefix=TARGET-19
 // RUN: %clang -target i686-pc-windows-msvc   -S -emit-llvm %s -o - -fms-compatibility-version=19 | FileCheck %s --check-prefix=OVERRIDE-19
 // RUN: %clang -target i686-pc-windows-msvc-elf -S -emit-llvm %s -o - | FileCheck %s --check-prefix=ELF-DEFAULT
 
-// DEFAULT: target triple = "i686-pc-windows-msvc18.0.0"
 // TARGET-19:   target triple = "i686-pc-windows-msvc19.0.0"
 // OVERRIDE-19: target triple = "i686-pc-windows-msvc19.0.0"
-// ELF-DEFAULT: target triple = "i686-pc-windows-msvc18.0.0-elf"
+// ELF-DEFAULT: target triple = "i686-pc-windows-msvc{{.*}}-elf"
Index: test/Driver/msc-version.c
===
--- test/Driver/msc-version.c
+++ test/Driver/msc-version.c
@@ -1,15 +1,4 @@
 //
-// Verify defaults
-//
-
-// RUN: %clang -target i686-windows -fms-compatibility -dM -E - &1 | FileCheck -check-prefix=NoThreadSafeStatics %s
+// RUN: %clang_cl /c -### -fms-compatibility-version=18 -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s
 // RUN: %clang_cl /Zc:threadSafeInit /Zc:threadSafeInit- /c -### -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s
 // NoThreadSafeStatics: "-fno-threadsafe-statics"
 
Index: lib/Driver/Tools.h
===
--- lib/Driver/Tools.h
+++ lib/Driver/Tools.h
@@ -682,7 +682,8 @@
 
 /// Visual studio tools.
 namespace visualstudio {
-VersionTuple getMSVCVersion(const Driver *D, const llvm::Triple ,
+VersionTuple getMSVCVersion(const Driver *D, const ToolChain ,
+const llvm::Triple ,
 const llvm::opt::ArgList , bool IsWindowsMSVC);
 
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -3291,7 +3291,7 @@
   Result.append(UID.begin(), UID.end());
 }
 
-VersionTuple visualstudio::getMSVCVersion(const Driver *D,
+VersionTuple visualstudio::getMSVCVersion(const Driver *D, const ToolChain ,
   const llvm::Triple ,
   const llvm::opt::ArgList ,
   bool IsWindowsMSVC) {
@@ -,8 +,14 @@
 if (Major || Minor || Micro)
   return VersionTuple(Major, Minor, Micro);
 
-// FIXME: Consider bumping this to 19 (MSVC2015) soon.
-return VersionTuple(18);
+if (IsWindowsMSVC) {
+  VersionTuple MSVT = TC.getMSVCVersionFromExe();
+  if (!MSVT.empty())
+return MSVT;
+
+  // FIXME: Consider bumping this to 19 (MSVC2015) soon.
+  return VersionTuple(18);
+}
   }
   return VersionTuple();
 }
@@ -5224,7 +5230,7 @@
 
   // -fms-compatibility-version=18.00 is default.
   VersionTuple MSVT = visualstudio::getMSVCVersion(
-  , getToolChain().getTriple(), Args, IsWindowsMSVC);
+  , getToolChain(), getToolChain().getTriple(), Args, IsWindowsMSVC);
   if (!MSVT.empty())
 CmdArgs.push_back(
 Args.MakeArgString("-fms-compatibility-version=" + MSVT.getAsString()));
Index: 

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

[Reid Kleckner via cfe-commits]

rnk added inline comments.


Comment at: lib/Driver/MSVCToolChain.cpp:41
@@ -40,1 +40,3 @@
+
+  #pragma comment(lib, "version.lib")
 #endif

Personally, I think this is OK but I know Aaron Ballman and other people don't 
like using pragma comment lib. The alternative is hacking CMake goo, which is 
always best avoided when possible.


Comment at: lib/Driver/MSVCToolChain.cpp:472
@@ +471,3 @@
+
+  const DWORD VersionSize = ::GetFileVersionInfoSizeA(ClExe.c_str(), nullptr);
+  if (VersionSize == 0) {

majnemer wrote:
> 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...
+1, you can use ConvertUTF8toWide to make this easy.


Comment at: lib/Driver/MSVCToolChain.cpp:477
@@ +476,3 @@
+  std::vector VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoA(ClExe.c_str(), 0, VersionSize,
+ VersionBlock.data())) {

majnemer wrote:
> 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).
I think one extra file read is probably worth the convenience it buys for our 
users. It's easy to win back by having the user pass an explicit 
-fms-compatibility-version flag.


Comment at: lib/Driver/Tools.cpp:3337-3338
@@ +3336,4 @@
+if (IsWindowsMSVC) {
+  const auto  = static_cast(TC);
+  VersionTuple MSVT = MSVC.getMSVCVersionFromExe();
+  if (!MSVT.empty())

IMO you should make this a virtual method on Toolchain that does nothing and is 
only overridden in MSVCToolChain. You can also cache it if you do that.


Comment at: tools/driver/driver.cpp:504
@@ -503,3 +503,3 @@
   // Exit status should not be negative on Win32, unless abnormal termination.
-  // Once abnormal termiation was caught, negative status should not be
+  // Once abnormal termination was caught, negative status should not be
   // propagated.

Yeah, it's a typo, but you don't have any other changes in this file, so I 
wouldn't touch it as part of this change.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

On Tue, May 10, 2016 at 6:09 PM, Adrian McCarthy via cfe-commits
 wrote:
> amccarth added inline comments.
>
> 
> Comment at: lib/Driver/MSVCToolChain.cpp:472
> @@ +471,3 @@
> +
> +  const DWORD VersionSize = ::GetFileVersionInfoSizeA(ClExe.c_str(), 
> nullptr);
> +  if (VersionSize == 0) {
> 
> 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.

Please use the W version instead of the A version. Not everyone
installs to the default path, and non-ASCII characters happen.

~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

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

[Adrian McCarthy via cfe-commits]

amccarth added inline comments.


Comment at: lib/Driver/MSVCToolChain.cpp:472
@@ +471,3 @@
+
+  const DWORD VersionSize = ::GetFileVersionInfoSizeA(ClExe.c_str(), nullptr);
+  if (VersionSize == 0) {

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.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

amccarth added inline comments.


Comment at: lib/Driver/MSVCToolChain.cpp:477
@@ +476,3 @@
+  std::vector VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoA(ClExe.c_str(), 0, VersionSize,
+ VersionBlock.data())) {

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.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Majnemer via cfe-commits]

majnemer added a subscriber: majnemer.


Comment at: lib/Driver/MSVCToolChain.cpp:472
@@ +471,3 @@
+
+  const DWORD VersionSize = ::GetFileVersionInfoSizeA(ClExe.c_str(), nullptr);
+  if (VersionSize == 0) {

Why not use the `GetFileVersionInfoSizeW` variant?


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via cfe-commits]

thakis added a subscriber: thakis.


Comment at: lib/Driver/MSVCToolChain.cpp:477
@@ +476,3 @@
+  std::vector VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoA(ClExe.c_str(), 0, VersionSize,
+ VersionBlock.data())) {

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.


http://reviews.llvm.org/D20136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits