On 23/06/2014 04:30, Saleem Abdulrasool wrote:
On Sun, Jun 22, 2014 at 4:15 PM, Alp Toker <[email protected]
<mailto:[email protected]>> wrote:
On 23/06/2014 01:53, Saleem Abdulrasool wrote:
On Sat, Jun 21, 2014 at 11:32 AM, Alp Toker <[email protected]
<mailto:[email protected]> <mailto:[email protected]
<mailto:[email protected]>>> wrote:
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:
I did this based on review feedback.
Hi Saleem,
I read the review thread and don't see any mention of
canonicalisation issues, or the bug where 17.0 and 17 are
mishandled. The review was mostly about coding style.
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140616/108136.html
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.
I wrote a detailed point-by-point review about how this doesn't
fit in with the design of our driver or expected usage -- could
you be more specific?
I was referring to the dot-separated value.
Accepting a dotted version is fine. Overloading the existing
-fmcs-version= option is the problem..
* Makes configure-time feature detection harder ("but the
option
exists, why doesn't it accept my input?")
This seems like it is a pre-existing problem, not something specific
to this change.
Nope, definitely specific to this change.
* Confuses argument canonicalization routines in external tools
like ccache, potentially requiring special handling to be
duplicated in each external project.
Can you provide some concrete example of this please? Im afraid I
dont see how that would happen.
External tools need to parse and grok driver arguments. Supporting two
syntaxes in a single option requires custom handling to be developed in
each tool -- there are very few other options like yours in the clang
driver. Where they exist, it's done out of necessity to support GCC bugs.
Your feature could easily be implemented as a second flag at no cost,
and that seems to be the obvious way to go. Is there a problem with
actually doing that?
* 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.
Not sure what you are referring to here. What IDE are you talking
about? The entire point of the compatibility format is that the old
format continues. You can provide it as an integral value.
* 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
I didnt think that we supported this previously, but, if it is simply
a matter of handling this case as well, I am more than happy to
accommodate that.
I don't see how you could accommodate that without adding more
disambiguations, checking for '.' characters or testing the input
length, or perhaps detecting if it's a very large or small number.
The base case where the input is integral will always conflict -- you
have no way to guess which one the user intended.
The quirks are already problematic and adding more isn't the solution
here: This could easily be made into two distinct options, and we'll
then be able to clearly document the newly-added option so users know
straight away if it's supported in their version of clang.
Alp.
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.
Switching to 64-bit would require additional surgery as the
current LangOptions are bit field and macro based. Changing
representation here would definitely help.
We're all agreed solving the representation will help here.
Given the inconsistent behaviour and sprinkling of FIXMEs I don't
think this patch is ready for ToT just yet.
Alp.
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 <tel:15.00.20706.01>
<tel:15.00.20706.01 <tel: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] <mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-- http://www.nuanti.com
the browser experts
--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
--
http://www.nuanti.com
the browser experts
--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
