r211420 - Driver: enhance MSC version compatibility
Alp Toker
alp at nuanti.com
Sun Jun 22 16:15:04 PDT 2014
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.
> 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?
> * 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.
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
More information about the cfe-commits
mailing list