<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Jun 21, 2014 at 11:32 AM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Late to the party with review but I have some points of review..<div class=""><br>
<br>
On 21/06/2014 01:58, Saleem Abdulrasool wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: compnerd<br>
Date: Fri Jun 20 17:58:35 2014<br>
New Revision: 211420<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=211420&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=211420&view=rev</a><br>
Log:<br>
Driver: enhance MSC version compatibility<br>
<br>
The version information for Visual Studio is spread over multiple variables.<br>
The newer Windows SDK has started making use of some of the extended versioning<br>
variables that were previously undefined. Enhance our compatibility definitions<br>
for these cases.<br>
<br>
_MSC_VER is defined to be the Major * 100 + Minor. _MSC_FULL_VER is defined to<br>
be Major * 10000000 + Minor * 100000 + Build. And _MSC_BUILD is the build<br>
revision of the compiler.<br>
<br>
Extend the -fmsc-version option in a compatible manner. If the value is the<br>
previous form of MMmm, then we assume that the build number is 0. Otherwise, a<br>
specific build number may be passed by using the form MMmmbbbbb.<br>
</blockquote>
<br></div>
We shouldn't overload the semantics of compiler options like this, because doing so:<br></blockquote><div><br></div><div>I did this based on review feedback. I have a slight preference for it, but its not a strong opinion. I would rather Reid and Aaron chime in and we decide on the path we wish to take before I spend effort to try to change the behaviour added here.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* Makes configure-time feature detection harder ("but the option exists, why doesn't it accept my input?")<br>
<br>
* Confuses argument canonicalization routines in external tools like ccache, potentially requiring special handling to be duplicated in each external project.<br>
<br>
* Breaks representation in IDE integration property pages: we'd like to use an integer mask for the numeric version form, and a dotted masked input selector in the GUI for fields like this. Overloading the syntax as in your patch means it can only be presented as an untyped string field.<br>
<br>
* Semantically inconsistent:<br>
<br>
-msc-version=17.0.0:<br>
#define _MSC_VER 1700<br>
<br>
-msc-version=17.0:<br>
#define _MSC_VER 1700<br>
<br>
-msc-version=17:<br>
#define _MSC_VER 17<br>
<br>
Oops!<br>
<br>
* Burden of documentation (Either does ... or ...)<br>
<br>
Please split out and name the new dotted form something like "-msc-full-version" to avoid ambiguity.<div class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Due to<br>
bitwidth limitations of the option, it is currently not possible to define a<br>
revision value.<br>
</blockquote>
<br></div>
What would be the practical burden of encoding this as 64-bits, or two 32-bit fields (one representing _MSC_BUILD and another for the rest)?<br>
<br>
Or is there a different plan to resolve the 32-bit encoding FIXMEs introduced in the commit? I suspect that getting the representation right will simplify computations below for free.</blockquote><div><br></div><div>Switching to 64-bit would require additional surgery as the current LangOptions are bit field and macro based. Changing representation here would definitely help.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The version information can be passed as either the decimal encoded value<br>
(_MSC_FULL_VER or _MSC_VER) or as a dot-delimited value.<br>
<br>
The change to the TextDiagnostic is to deal with the updated encoding of the<br>
version information.<br>
<br>
Added:<br>
cfe/trunk/test/Driver/msc-<u></u>version.c<br>
Modified:<br>
cfe/trunk/lib/Basic/Targets.<u></u>cpp<br>
cfe/trunk/lib/Frontend/<u></u>CompilerInvocation.cpp<br>
cfe/trunk/lib/Frontend/<u></u>TextDiagnostic.cpp<br>
<br>
Modified: cfe/trunk/lib/Basic/Targets.<u></u>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=211420&r1=211419&r2=211420&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/lib/Basic/<u></u>Targets.cpp?rev=211420&r1=<u></u>211419&r2=211420&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/lib/Basic/Targets.<u></u>cpp (original)<br>
+++ cfe/trunk/lib/Basic/Targets.<u></u>cpp Fri Jun 20 17:58:35 2014<br>
@@ -587,8 +587,12 @@ protected:<br>
if (Opts.POSIXThreads)<br>
Builder.defineMacro("_MT");<br>
- if (Opts.MSCVersion != 0)<br>
- Builder.defineMacro("_MSC_VER"<u></u>, Twine(Opts.MSCVersion));<br>
+ if (Opts.MSCVersion != 0) {<br>
+ Builder.defineMacro("_MSC_VER"<u></u>, Twine(Opts.MSCVersion / 100000));<br>
+ Builder.defineMacro("_MSC_<u></u>FULL_VER", Twine(Opts.MSCVersion));<br>
+ // FIXME We cannot encode the revision information into 32-bits<br>
+ Builder.defineMacro("_MSC_<u></u>BUILD", Twine(1));<br>
+ }<br>
if (Opts.MicrosoftExt) {<br>
Builder.defineMacro("_MSC_<u></u>EXTENSIONS");<br>
<br>
Modified: cfe/trunk/lib/Frontend/<u></u>CompilerInvocation.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=211420&r1=211419&r2=211420&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/lib/<u></u>Frontend/CompilerInvocation.<u></u>cpp?rev=211420&r1=211419&r2=<u></u>211420&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/lib/Frontend/<u></u>CompilerInvocation.cpp (original)<br>
+++ cfe/trunk/lib/Frontend/<u></u>CompilerInvocation.cpp Fri Jun 20 17:58:35 2014<br>
@@ -20,6 +20,7 @@<br>
#include "clang/Serialization/<u></u>ASTReader.h"<br>
#include "llvm/ADT/Hashing.h"<br>
#include "llvm/ADT/SmallVector.h"<br>
+#include "llvm/ADT/STLExtras.h"<br>
#include "llvm/ADT/StringExtras.h"<br>
#include "llvm/ADT/StringSwitch.h"<br>
#include "llvm/ADT/Triple.h"<br>
@@ -1194,6 +1195,56 @@ static Visibility parseVisibility(Arg *a<br>
return DefaultVisibility;<br>
}<br>
+static unsigned parseMSCVersion(ArgList &Args, DiagnosticsEngine &Diags) {<br>
+ auto Arg = Args.getLastArg(OPT_fmsc_<u></u>version);<br>
+ if (!Arg)<br>
+ return 0;<br>
+<br>
+ // The MSC versioning scheme involves four versioning components:<br>
+ // - Major<br>
+ // - Minor<br>
+ // - Build<br>
+ // - Patch<br>
+ //<br>
+ // We accept either the old style (_MSC_VER) value, or a _MSC_FULL_VER value.<br>
+ // Additionally, the value may be provided in the form of a more readable<br>
+ // MM.mm.bbbbb.pp version.<br>
+ //<br>
+ // Unfortunately, due to the bit-width limitations, we cannot currently encode<br>
+ // the value for the patch level.<br>
+<br>
+ StringRef Value = Arg->getValue();<br>
+<br>
+ // parse the compatible old form of _MSC_VER or the newer _MSC_FULL_VER<br>
+ if (Value.find('.') == StringRef::npos) {<br>
+ unsigned Version = 0;<br>
+ if (Value.getAsInteger(10, Version)) {<br>
+ Diags.Report(diag::err_drv_<u></u>invalid_value)<br>
+ << Arg->getAsString(Args) << Value;<br>
+ return 0;<br>
+ }<br>
+ return (Version < 100000) ? Version * 100000 : Version;<br>
+ }<br>
+<br>
+ // parse the dot-delimited component version<br>
+ unsigned VC[4] = {0};<br>
+ SmallVector<StringRef, 4> Components;<br>
+<br>
+ Value.split(Components, ".", llvm::array_lengthof(VC));<br>
+ for (unsigned CI = 0,<br>
+ CE = std::min(Components.size(), llvm::array_lengthof(VC));<br>
+ CI < CE; ++CI) {<br>
+ if (Components[CI].getAsInteger(<u></u>10, VC[CI])) {<br>
+ Diags.Report(diag::err_drv_<u></u>invalid_value)<br>
+ << Arg->getAsString(Args) << Value;<br>
+ return 0;<br>
+ }<br>
+ }<br>
+<br>
+ // FIXME we cannot encode the patch level<br>
+ return VC[0] * 10000000 + VC[1] * 100000 + VC[2];<br>
+}<br>
+<br>
static void ParseLangArgs(LangOptions &Opts, ArgList &Args, InputKind IK,<br>
DiagnosticsEngine &Diags) {<br>
// FIXME: Cleanup per-file based stuff.<br>
@@ -1363,7 +1414,7 @@ static void ParseLangArgs(LangOptions &O<br>
Opts.MSVCCompat = Args.hasArg(OPT_fms_<u></u>compatibility);<br>
Opts.MicrosoftExt = Opts.MSVCCompat || Args.hasArg(OPT_fms_<u></u>extensions);<br>
Opts.AsmBlocks = Args.hasArg(OPT_fasm_blocks) || Opts.MicrosoftExt;<br>
- Opts.MSCVersion = getLastArgIntValue(Args, OPT_fmsc_version, 0, Diags);<br>
+ Opts.MSCVersion = parseMSCVersion(Args, Diags);<br>
Opts.VtorDispMode = getLastArgIntValue(Args, OPT_vtordisp_mode_EQ, 1, Diags);<br>
Opts.Borland = Args.hasArg(OPT_fborland_<u></u>extensions);<br>
Opts.WritableStrings = Args.hasArg(OPT_fwritable_<u></u>strings);<br>
<br>
Modified: cfe/trunk/lib/Frontend/<u></u>TextDiagnostic.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/TextDiagnostic.cpp?rev=211420&r1=211419&r2=211420&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/lib/<u></u>Frontend/TextDiagnostic.cpp?<u></u>rev=211420&r1=211419&r2=<u></u>211420&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/lib/Frontend/<u></u>TextDiagnostic.cpp (original)<br>
+++ cfe/trunk/lib/Frontend/<u></u>TextDiagnostic.cpp Fri Jun 20 17:58:35 2014<br>
@@ -813,7 +813,7 @@ void TextDiagnostic::<u></u>emitDiagnosticLoc(S<br>
if (DiagOpts->getFormat() == DiagnosticOptions::Msvc) {<br>
OS << ',';<br>
// Visual Studio 2010 or earlier expects column number to be off by one<br>
- if (LangOpts.MSCVersion && LangOpts.MSCVersion < 1700)<br>
+ if (LangOpts.MSCVersion && LangOpts.MSCVersion < 170000000)<br>
ColNo--;<br>
</blockquote>
<br></div></div>
(Wow, not your fault but this code requiring macro pre-defines just to influence diagnostic formatting for IDE-parsability is wrong on so many levels.)<div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
} else<br>
OS << ':';<br>
<br>
Added: cfe/trunk/test/Driver/msc-<u></u>version.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/msc-version.c?rev=211420&view=auto" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/test/Driver/<u></u>msc-version.c?rev=211420&view=<u></u>auto</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/test/Driver/msc-<u></u>version.c (added)<br>
+++ cfe/trunk/test/Driver/msc-<u></u>version.c Fri Jun 20 17:58:35 2014<br>
@@ -0,0 +1,36 @@<br>
+// RUN: %clang -target i686-windows -fms-compatibility -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-NO-MSC-VERSION<br>
+<br>
+// CHECK-NO-MSC-VERSION: _MSC_BUILD 1<br>
+// CHECK-NO-MSC-VERSION: _MSC_FULL_VER 170000000<br>
+// CHECK-NO-MSC-VERSION: _MSC_VER 1700<br>
+<br>
+// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=1600 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION<br>
+<br>
+// CHECK-MSC-VERSION: _MSC_BUILD 1<br>
+// CHECK-MSC-VERSION: _MSC_FULL_VER 160000000<br>
+// CHECK-MSC-VERSION: _MSC_VER 1600<br>
+<br>
+// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=160030319 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-EXT<br>
+<br>
+// CHECK-MSC-VERSION-EXT: _MSC_BUILD 1<br>
+// CHECK-MSC-VERSION-EXT: _MSC_FULL_VER 160030319<br>
+// CHECK-MSC-VERSION-EXT: _MSC_VER 1600<br>
+<br>
+// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=17.00 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR<br>
</blockquote>
<br></div></div>
Test will be needed for the following, which currently does the unexpected thing:<br>
<br>
// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=17 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR<br>
<br>
Alp.<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_BUILD 1<br>
+// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_FULL_VER 170000000<br>
+// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_VER 1700<br>
+<br>
+// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=15.00.20706 -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR-<u></u>BUILD<br>
+<br>
+// CHECK-MSC-VERSION-MAJOR-MINOR-<u></u>BUILD: _MSC_BUILD 1<br>
+// CHECK-MSC-VERSION-MAJOR-MINOR-<u></u>BUILD: _MSC_FULL_VER 150020706<br>
+// CHECK-MSC-VERSION-MAJOR-MINOR-<u></u>BUILD: _MSC_VER 1500<br>
+<br>
+// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=<a href="tel:15.00.20706.01" value="+15002070601" target="_blank">15.00.20706.01</a> -dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR-<u></u>BUILD-PATCH<br>
+<br>
+// CHECK-MSC-VERSION-MAJOR-MINOR-<u></u>BUILD-PATCH: _MSC_BUILD 1<br>
+// CHECK-MSC-VERSION-MAJOR-MINOR-<u></u>BUILD-PATCH: _MSC_FULL_VER 150020706<br>
+// CHECK-MSC-VERSION-MAJOR-MINOR-<u></u>BUILD-PATCH: _MSC_VER 1500<br>
+<br>
<br>
<br>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
</blockquote>
<br></div></div><span class="HOEnZb"><font color="#888888">
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br>Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org
</div></div>