r211420 - Driver: enhance MSC version compatibility
Alp Toker
alp at nuanti.com
Sat Jun 21 11:32:08 PDT 2014
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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list