r211420 - Driver: enhance MSC version compatibility
Saleem Abdulrasool
compnerd at compnerd.org
Sun Jun 22 18:30:32 PDT 2014
On Sun, Jun 22, 2014 at 4:15 PM, Alp Toker <alp at nuanti.com> wrote:
>
> On 23/06/2014 01:53, Saleem Abdulrasool wrote:
>
> On Sat, Jun 21, 2014 at 11:32 AM, Alp Toker <alp at nuanti.com <mailto:
>> alp at nuanti.com>> 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.
>
> * 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.
> * 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.
> * 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.
> 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> -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
>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>
>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140622/32adf1fe/attachment.html>
More information about the cfe-commits
mailing list