Late to the party with review but I have some points of review..
On 21/06/2014 01:58, Saleem Abdulrasool wrote:
Author: compnerd
Date: Fri Jun 20 17:58:35 2014
New Revision: 211420
URL: http://llvm.org/viewvc/llvm-project?rev=211420&view=rev
Log:
Driver: enhance MSC version compatibility
The version information for Visual Studio is spread over multiple variables.
The newer Windows SDK has started making use of some of the extended versioning
variables that were previously undefined. Enhance our compatibility definitions
for these cases.
_MSC_VER is defined to be the Major * 100 + Minor. _MSC_FULL_VER is defined to
be Major * 10000000 + Minor * 100000 + Build. And _MSC_BUILD is the build
revision of the compiler.
Extend the -fmsc-version option in a compatible manner. If the value is the
previous form of MMmm, then we assume that the build number is 0. Otherwise, a
specific build number may be passed by using the form MMmmbbbbb.
We shouldn't overload the semantics of compiler options like this,
because doing so:
* Makes configure-time feature detection harder ("but the option
exists, why doesn't it accept my input?")
* Confuses argument canonicalization routines in external tools like
ccache, potentially requiring special handling to be duplicated in each
external project.
* 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.
* Semantically inconsistent:
-msc-version=17.0.0:
#define _MSC_VER 1700
-msc-version=17.0:
#define _MSC_VER 1700
-msc-version=17:
#define _MSC_VER 17
Oops!
* Burden of documentation (Either does ... or ...)
Please split out and name the new dotted form something like
"-msc-full-version" to avoid ambiguity.
Due to
bitwidth limitations of the option, it is currently not possible to define a
revision value.
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)?
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.
The version information can be passed as either the decimal encoded value
(_MSC_FULL_VER or _MSC_VER) or as a dot-delimited value.
The change to the TextDiagnostic is to deal with the updated encoding of the
version information.
Added:
cfe/trunk/test/Driver/msc-version.c
Modified:
cfe/trunk/lib/Basic/Targets.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/lib/Frontend/TextDiagnostic.cpp
Modified: cfe/trunk/lib/Basic/Targets.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=211420&r1=211419&r2=211420&view=diff
==============================================================================
--- cfe/trunk/lib/Basic/Targets.cpp (original)
+++ cfe/trunk/lib/Basic/Targets.cpp Fri Jun 20 17:58:35 2014
@@ -587,8 +587,12 @@ protected:
if (Opts.POSIXThreads)
Builder.defineMacro("_MT");
- if (Opts.MSCVersion != 0)
- Builder.defineMacro("_MSC_VER", Twine(Opts.MSCVersion));
+ if (Opts.MSCVersion != 0) {
+ Builder.defineMacro("_MSC_VER", Twine(Opts.MSCVersion / 100000));
+ Builder.defineMacro("_MSC_FULL_VER", Twine(Opts.MSCVersion));
+ // FIXME We cannot encode the revision information into 32-bits
+ Builder.defineMacro("_MSC_BUILD", Twine(1));
+ }
if (Opts.MicrosoftExt) {
Builder.defineMacro("_MSC_EXTENSIONS");
Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=211420&r1=211419&r2=211420&view=diff
==============================================================================
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Fri Jun 20 17:58:35 2014
@@ -20,6 +20,7 @@
#include "clang/Serialization/ASTReader.h"
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/ADT/Triple.h"
@@ -1194,6 +1195,56 @@ static Visibility parseVisibility(Arg *a
return DefaultVisibility;
}
+static unsigned parseMSCVersion(ArgList &Args, DiagnosticsEngine &Diags) {
+ auto Arg = Args.getLastArg(OPT_fmsc_version);
+ if (!Arg)
+ return 0;
+
+ // The MSC versioning scheme involves four versioning components:
+ // - Major
+ // - Minor
+ // - Build
+ // - Patch
+ //
+ // We accept either the old style (_MSC_VER) value, or a _MSC_FULL_VER value.
+ // Additionally, the value may be provided in the form of a more readable
+ // MM.mm.bbbbb.pp version.
+ //
+ // Unfortunately, due to the bit-width limitations, we cannot currently
encode
+ // the value for the patch level.
+
+ StringRef Value = Arg->getValue();
+
+ // parse the compatible old form of _MSC_VER or the newer _MSC_FULL_VER
+ if (Value.find('.') == StringRef::npos) {
+ unsigned Version = 0;
+ if (Value.getAsInteger(10, Version)) {
+ Diags.Report(diag::err_drv_invalid_value)
+ << Arg->getAsString(Args) << Value;
+ return 0;
+ }
+ return (Version < 100000) ? Version * 100000 : Version;
+ }
+
+ // parse the dot-delimited component version
+ unsigned VC[4] = {0};
+ SmallVector<StringRef, 4> Components;
+
+ Value.split(Components, ".", llvm::array_lengthof(VC));
+ for (unsigned CI = 0,
+ CE = std::min(Components.size(), llvm::array_lengthof(VC));
+ CI < CE; ++CI) {
+ if (Components[CI].getAsInteger(10, VC[CI])) {
+ Diags.Report(diag::err_drv_invalid_value)
+ << Arg->getAsString(Args) << Value;
+ return 0;
+ }
+ }
+
+ // FIXME we cannot encode the patch level
+ return VC[0] * 10000000 + VC[1] * 100000 + VC[2];
+}
+
static void ParseLangArgs(LangOptions &Opts, ArgList &Args, InputKind IK,
DiagnosticsEngine &Diags) {
// FIXME: Cleanup per-file based stuff.
@@ -1363,7 +1414,7 @@ static void ParseLangArgs(LangOptions &O
Opts.MSVCCompat = Args.hasArg(OPT_fms_compatibility);
Opts.MicrosoftExt = Opts.MSVCCompat || Args.hasArg(OPT_fms_extensions);
Opts.AsmBlocks = Args.hasArg(OPT_fasm_blocks) || Opts.MicrosoftExt;
- Opts.MSCVersion = getLastArgIntValue(Args, OPT_fmsc_version, 0, Diags);
+ Opts.MSCVersion = parseMSCVersion(Args, Diags);
Opts.VtorDispMode = getLastArgIntValue(Args, OPT_vtordisp_mode_EQ, 1,
Diags);
Opts.Borland = Args.hasArg(OPT_fborland_extensions);
Opts.WritableStrings = Args.hasArg(OPT_fwritable_strings);
Modified: cfe/trunk/lib/Frontend/TextDiagnostic.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/TextDiagnostic.cpp?rev=211420&r1=211419&r2=211420&view=diff
==============================================================================
--- cfe/trunk/lib/Frontend/TextDiagnostic.cpp (original)
+++ cfe/trunk/lib/Frontend/TextDiagnostic.cpp Fri Jun 20 17:58:35 2014
@@ -813,7 +813,7 @@ void TextDiagnostic::emitDiagnosticLoc(S
if (DiagOpts->getFormat() == DiagnosticOptions::Msvc) {
OS << ',';
// Visual Studio 2010 or earlier expects column number to be off by
one
- if (LangOpts.MSCVersion && LangOpts.MSCVersion < 1700)
+ if (LangOpts.MSCVersion && LangOpts.MSCVersion < 170000000)
ColNo--;
(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.)
} else
OS << ':';
Added: cfe/trunk/test/Driver/msc-version.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/msc-version.c?rev=211420&view=auto
==============================================================================
--- cfe/trunk/test/Driver/msc-version.c (added)
+++ cfe/trunk/test/Driver/msc-version.c Fri Jun 20 17:58:35 2014
@@ -0,0 +1,36 @@
+// RUN: %clang -target i686-windows -fms-compatibility -dM -E - </dev/null -o
- | FileCheck %s -check-prefix CHECK-NO-MSC-VERSION
+
+// CHECK-NO-MSC-VERSION: _MSC_BUILD 1
+// CHECK-NO-MSC-VERSION: _MSC_FULL_VER 170000000
+// CHECK-NO-MSC-VERSION: _MSC_VER 1700
+
+// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=1600 -dM -E
- </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION
+
+// CHECK-MSC-VERSION: _MSC_BUILD 1
+// CHECK-MSC-VERSION: _MSC_FULL_VER 160000000
+// CHECK-MSC-VERSION: _MSC_VER 1600
+
+// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=160030319
-dM -E - </dev/null -o - | FileCheck %s -check-prefix CHECK-MSC-VERSION-EXT
+
+// CHECK-MSC-VERSION-EXT: _MSC_BUILD 1
+// CHECK-MSC-VERSION-EXT: _MSC_FULL_VER 160030319
+// CHECK-MSC-VERSION-EXT: _MSC_VER 1600
+
+// 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
Test will be needed for the following, which currently does the
unexpected thing:
// RUN: %clang -target i686-windows -fms-compatibility -fmsc-version=17
-dM -E - </dev/null -o - | FileCheck %s -check-prefix
CHECK-MSC-VERSION-MAJOR-MINOR
Alp.
+
+// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_BUILD 1
+// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_FULL_VER 170000000
+// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_VER 1700
+
+// 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-BUILD
+
+// CHECK-MSC-VERSION-MAJOR-MINOR-BUILD: _MSC_BUILD 1
+// CHECK-MSC-VERSION-MAJOR-MINOR-BUILD: _MSC_FULL_VER 150020706
+// CHECK-MSC-VERSION-MAJOR-MINOR-BUILD: _MSC_VER 1500
+
+// RUN: %clang -target i686-windows -fms-compatibility
-fmsc-version=15.00.20706.01 -dM -E - </dev/null -o - | FileCheck %s
-check-prefix CHECK-MSC-VERSION-MAJOR-MINOR-BUILD-PATCH
+
+// CHECK-MSC-VERSION-MAJOR-MINOR-BUILD-PATCH: _MSC_BUILD 1
+// CHECK-MSC-VERSION-MAJOR-MINOR-BUILD-PATCH: _MSC_FULL_VER 150020706
+// CHECK-MSC-VERSION-MAJOR-MINOR-BUILD-PATCH: _MSC_VER 1500
+
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
