r211420 - Driver: enhance MSC version compatibility
Saleem Abdulrasool
compnerd at compnerd.org
Sun Jun 22 15:53:02 PDT 2014
On Sat, Jun 21, 2014 at 11:32 AM, Alp Toker <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. 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.
> * 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.
Switching to 64-bit would require additional surgery as the current
LangOptions are bit field and macro based. Changing representation here
would definitely help.
>
>
>> 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
>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140622/01f3926e/attachment.html>
More information about the cfe-commits
mailing list