[PATCH] Driver: bifurcate extended and basic MSC versioning
Reid Kleckner
rnk at google.com
Tue Jul 1 11:50:41 PDT 2014
On Tue, Jul 1, 2014 at 8:58 AM, Aaron Ballman <aaron.ballman at gmail.com>
wrote:
> On Tue, Jul 1, 2014 at 1:15 AM, Saleem Abdulrasool
> <compnerd at compnerd.org> wrote:
> > Hi rnk, aaron.ballman,
> >
> > This restores the original behaviour of `-fmsc-version`. The older
> option
> > remains as a mechanism for specifying the basic version information. A
> > secondary option, `-fmsc-version-ex` permits the user to specify an
> extended
> > version to the driver.
> >
> > The new version takes the value as a dot-separated value rather than the
> > major * 100 + minor format that `-fmsc-version` format. This makes it
> easier to
> > specify the value as well as a more flexible manner for specifying the
> value.
> >
> > Specifying both values is considered an error (TODO: add test case).
> The driver
> > gives preference to the extended format, defaulting to that format
> unless the
> > old option is specified.
>
> > Index: include/clang/Basic/LangOptions.def
> > ===================================================================
> > --- include/clang/Basic/LangOptions.def
> > +++ include/clang/Basic/LangOptions.def
> > @@ -181,6 +181,7 @@
> > "if non-zero, warn about parameter or return Warn if
> parameter/return value is larger in bytes than this setting. 0 is no
> check.")
> > VALUE_LANGOPT(MSCVersion, 32, 0,
> > "version of Microsoft Visual C/C++")
> > +VALUE_LANGOPT(MSCVersionEx, 32, 0, "Microsoft Visual C/C++ Extended
> Version")
>
> Is there a way we could get away with just one langopt member
> variable? I'm not keen on seeing code that has to check both variables
> to get the answer.
>
IMO we should tablegen-ify this, so that we can sort the options by size to
achieve better packing. When we do that, we can use uint64_t as the
bitfield type for options larger than 32 bits. Until then, two 32-bit
options is the best we can do.
Besides, do you think anyone will care about the build number in Clang?
Most language changes are introduced on new releases captured in the major
version.
> If we do have to keep both variables, I would prefer a better name
> than "Ex" (though I do enjoy the joke). Also, if both are needed, can
> we normalize the text to be similar between both options?
>
> > VALUE_LANGOPT(VtorDispMode, 2, 1, "How many vtordisps to insert")
> >
> > LANGOPT(ApplePragmaPack, 1, 0, "Apple gcc-compatible #pragma pack
> handling")
> > Index: include/clang/Driver/Options.td
> > ===================================================================
> > --- include/clang/Driver/Options.td
> > +++ include/clang/Driver/Options.td
> > @@ -577,6 +577,8 @@
> > HelpText<"Enable full Microsoft Visual C++ compatibility">;
> > def fmsc_version : Joined<["-"], "fmsc-version=">, Group<f_Group>,
> Flags<[CC1Option, CoreOption]>,
>
You can remove this as a -cc1 option probably, now that the driver
canonicalizes as -fmsc-full-version.
> > HelpText<"Microsoft compiler version number to report in _MSC_VER (0
> = don't define it (default))">;
> > +def fmsc_version_ex : Joined<["-"], "fmsc-version-ex=">, Group<f_Group>,
> > + Flags<[CC1Option, CoreOption]>, HelpText<"Extended Microsoft
> compiler version">;
>
> I think the public-facing name needs to change to something more
> descriptive. How about -fmsc-version-full? Also, I'd like the help
> text normalized between the two options.
>
+1 for -fmsc-full-version.
> > def fdelayed_template_parsing : Flag<["-"],
> "fdelayed-template-parsing">, Group<f_Group>,
> > HelpText<"Parse templated function definitions at the end of the "
> > "translation unit">, Flags<[CC1Option]>;
> > Index: lib/Basic/Targets.cpp
> > ===================================================================
> > --- lib/Basic/Targets.cpp
> > +++ lib/Basic/Targets.cpp
> > @@ -587,11 +587,13 @@
> > if (Opts.POSIXThreads)
> > Builder.defineMacro("_MT");
> >
> > - if (Opts.MSCVersion != 0) {
> > - Builder.defineMacro("_MSC_VER", Twine(Opts.MSCVersion / 100000));
> > - Builder.defineMacro("_MSC_FULL_VER", Twine(Opts.MSCVersion));
> > + if (Opts.MSCVersionEx) {
> > + Builder.defineMacro("_MSC_VER", Twine(Opts.MSCVersionEx /
> 100000));
> > + Builder.defineMacro("_MSC_FULL_VER", Twine(Opts.MSCVersionEx));
> > // FIXME We cannot encode the revision information into 32-bits
> > Builder.defineMacro("_MSC_BUILD", Twine(1));
> > + } else if (Opts.MSCVersion != 0) {
>
> Can drop the != 0 like you do for the Ex version.
>
> > + Builder.defineMacro("_MSC_VER", Twine(Opts.MSCVersion));
>
> Can we not set the full version as well (just missing the information
> we do not have). Same for _MSC_BUILD.
>
> I think it would be surprising for one of these to exist, but not all
> three.
>
> > }
> >
> > if (Opts.MicrosoftExt) {
> > Index: lib/Driver/Tools.cpp
> > ===================================================================
> > --- lib/Driver/Tools.cpp
> > +++ lib/Driver/Tools.cpp
> > @@ -3723,14 +3723,29 @@
> > true))))
> > CmdArgs.push_back("-fms-compatibility");
> >
> > - // -fmsc-version=1700 is default.
> > + // -fmsc-version-ex=17.00 is default.
> > if (Args.hasFlag(options::OPT_fms_extensions,
> options::OPT_fno_ms_extensions,
> > - IsWindowsMSVC) ||
> Args.hasArg(options::OPT_fmsc_version)) {
> > - StringRef msc_ver = Args.getLastArgValue(options::OPT_fmsc_version);
> > - if (msc_ver.empty())
> > - CmdArgs.push_back("-fmsc-version=1700");
> > - else
> > - CmdArgs.push_back(Args.MakeArgString("-fmsc-version=" + msc_ver));
> > + IsWindowsMSVC) ||
> Args.hasArg(options::OPT_fmsc_version) ||
> > + Args.hasArg(options::OPT_fmsc_version_ex)) {
> > + if (Args.hasArg(options::OPT_fmsc_version) &&
> > + Args.hasArg(options::OPT_fmsc_version_ex))
> > + D.Diag(diag::err_drv_argument_not_allowed_with)
> > + << Args.getArgString(options::OPT_fmsc_version)
> > + << Args.getArgString(options::OPT_fmsc_version_ex);
> > +
> > + if (Args.hasArg(options::OPT_fmsc_version_ex)) {
> > + StringRef MSCVerEx =
> Args.getLastArgValue(options::OPT_fmsc_version_ex);
> + if (MSCVerEx.empty())
> > + CmdArgs.push_back("-fmsc-version-ex=17.00");
>
This if branch and the other like it is probably unnecessary.
The logic should be:
if (Arg *A = Args.getLastArg(OPT_fmsc_full_version)) { ... }
else if (Arg *A = Args.getLastArg(OPT_fmsc_version)) { ... }
else { CmdArgs.push_back("-fmsc-full-version=17.00"); }
> > + else
> > + CmdArgs.push_back(Args.MakeArgString("-fmsc-version-ex=" +
> MSCVerEx));
> > + } else {
> > + StringRef MSCVer =
> Args.getLastArgValue(options::OPT_fmsc_version);
> > + if (MSCVer.empty())
> > + CmdArgs.push_back("-fmsc-version-ex=17.00");
> > + else
> > + CmdArgs.push_back(Args.MakeArgString("-fmsc-version=" +
> MSCVer));
> > + }
> > }
> >
> >
> > Index: lib/Frontend/CompilerInvocation.cpp
> > ===================================================================
> > --- lib/Frontend/CompilerInvocation.cpp
> > +++ lib/Frontend/CompilerInvocation.cpp
> > @@ -1208,7 +1208,7 @@
> > }
> >
> > static unsigned parseMSCVersion(ArgList &Args, DiagnosticsEngine
> &Diags) {
> > - auto Arg = Args.getLastArg(OPT_fmsc_version);
> > + auto Arg = Args.getLastArg(OPT_fmsc_version_ex);
> > if (!Arg)
> > return 0;
> >
> > @@ -1225,25 +1225,8 @@
> > // 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;
> > - }
> > - if (Version < 100)
> > - Version = Version * 100; // major -> major.minor
> > - if (Version < 100000)
> > - Version = Version * 100000; // major.minor -> major.minor.build
> > - return Version;
> > - }
> > -
> > - // parse the dot-delimited component version
> > unsigned VC[4] = {0};
> > + StringRef Value = Arg->getValue();
> > SmallVector<StringRef, 4> Components;
> >
> > Value.split(Components, ".", llvm::array_lengthof(VC));
> > @@ -1430,7 +1413,8 @@
> > 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 = parseMSCVersion(Args, Diags);
> > + Opts.MSCVersion = getLastArgIntValue(Args, OPT_fmsc_version, 0,
> Diags);
> > + Opts.MSCVersionEx = 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);
> > Index: lib/Frontend/TextDiagnostic.cpp
> > ===================================================================
> > --- lib/Frontend/TextDiagnostic.cpp
> > +++ lib/Frontend/TextDiagnostic.cpp
> > @@ -808,7 +808,8 @@
> > if (DiagOpts->getFormat() == DiagnosticOptions::Msvc) {
> > OS << ',';
> > // Visual Studio 2010 or earlier expects column number to be
> off by one
> > - if (LangOpts.MSCVersion && LangOpts.MSCVersion < 170000000)
> > + if ((LangOpts.MSCVersion && LangOpts.MSCVersion < 1700) ||
> > + (LangOpts.MSCVersionEx && LangOpts.MSCVersionEx <
> 170000000))
> > ColNo--;
> > } else
> > OS << ':';
> > Index: test/Driver/msc-version.c
> > ===================================================================
> > --- test/Driver/msc-version.c
> > +++ test/Driver/msc-version.c
> > @@ -4,37 +4,25 @@
> > // 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=14
> -dM -E - </dev/null -o - | FileCheck %s -check-prefix
> CHECK-MSC-VERSION-MAJOR
> > +// RUN: %clang -target i686-windows -fms-compatibility
> -fmsc-version-ex=14 -dM -E - </dev/null -o - | FileCheck %s -check-prefix
> CHECK-MSC-VERSION-MAJOR
> >
> > // CHECK-MSC-VERSION-MAJOR: _MSC_BUILD 1
> > // CHECK-MSC-VERSION-MAJOR: _MSC_FULL_VER 140000000
> > // CHECK-MSC-VERSION-MAJOR: _MSC_VER 1400
> >
> > -// 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
> > +// RUN: %clang -target i686-windows -fms-compatibility
> -fmsc-version-ex=15.00 -dM -E - </dev/null -o - | FileCheck %s
> -check-prefix CHECK-MSC-VERSION-MAJOR-MINOR
> >
> > // 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
> > +// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_FULL_VER 150000000
> > +// CHECK-MSC-VERSION-MAJOR-MINOR: _MSC_VER 1500
> >
> > -// 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
> > +// RUN: %clang -target i686-windows -fms-compatibility
> -fmsc-version-ex=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
> > +// RUN: %clang -target i686-windows -fms-compatibility
> -fmsc-version-ex=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
> > Index: test/Misc/diag-format.c
> > ===================================================================
> > --- test/Misc/diag-format.c
> > +++ test/Misc/diag-format.c
> > @@ -3,10 +3,13 @@
> > // RUN: %clang -fsyntax-only -fdiagnostics-format=clang -target
> x86_64-pc-win32 %s 2>&1 | FileCheck %s -check-prefix=DEFAULT
> > //
> > // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc
> -fmsc-version=1300 %s 2>&1 | FileCheck %s -check-prefix=MSVC2010
> > +// RUN: %clang -fsyntax-only -fdiagnostics-format=msvc
> -fmsc-version-ex=13.00 %s 2>&1 | FileCheck %s -check-prefix=MSVC2010
> > // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc %s 2>&1 |
> FileCheck %s -check-prefix=MSVC
> > // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc
> -fmsc-version=1300 -target x86_64-pc-win32 %s 2>&1 | FileCheck %s
> -check-prefix=MSVC2010
> > +// RUN: %clang -fsyntax-only -fdiagnostics-format=msvc
> -fmsc-version-ex=13.00 -target x86_64-pc-win32 %s 2>&1 | FileCheck %s
> -check-prefix=MSVC2010
> > // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -target
> x86_64-pc-win32 %s 2>&1 | FileCheck %s -check-prefix=MSVC
> > // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc
> -fmsc-version=1300 -target x86_64-pc-win32 -fshow-column %s 2>&1 |
> FileCheck %s -check-prefix=MSVC2010
> > +// RUN: %clang -fsyntax-only -fdiagnostics-format=msvc
> -fmsc-version-ex=13.00 -target x86_64-pc-win32 -fshow-column %s 2>&1 |
> FileCheck %s -check-prefix=MSVC2010
> > // RUN: %clang -fsyntax-only -fdiagnostics-format=msvc -target
> x86_64-pc-win32 -fshow-column %s 2>&1 | FileCheck %s -check-prefix=MSVC
> > //
> > // RUN: %clang -fsyntax-only -fdiagnostics-format=vi %s 2>&1 |
> FileCheck %s -check-prefix=VI
> > @@ -16,6 +19,7 @@
> > // RUN: %clang -fsyntax-only -fno-show-column %s 2>&1 | FileCheck %s
> -check-prefix=NO_COLUMN
> > //
> > // RUN: not %clang -fsyntax-only -Werror
> -fdiagnostics-format=msvc-fallback -fmsc-version=1300 %s 2>&1 | FileCheck
> %s -check-prefix=MSVC2010-FALLBACK
> > +// RUN: not %clang -fsyntax-only -Werror
> -fdiagnostics-format=msvc-fallback -fmsc-version-ex=13.00 %s 2>&1 |
> FileCheck %s -check-prefix=MSVC2010-FALLBACK
> > // RUN: not %clang -fsyntax-only -Werror
> -fdiagnostics-format=msvc-fallback %s 2>&1 | FileCheck %s
> -check-prefix=MSVC-FALLBACK
> >
> >
> > @@ -30,12 +34,12 @@
> >
> > #ifdef foo
> > #endif bad // extension!
> > -// DEFAULT: {{.*}}:32:8: warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > -// MSVC2010: {{.*}}(32,7) : warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > -// MSVC: {{.*}}(32,8) : warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > -// VI: {{.*}} +32:8: warning: extra tokens at end of #endif directive
> [-Wextra-tokens]
> > -// MSVC_ORIG: {{.*}}(32) : warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > -// NO_COLUMN: {{.*}}:32: warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > -// MSVC2010-FALLBACK: {{.*}}(32,7) : error(clang): extra tokens at end
> of #endif directive
> > -// MSVC-FALLBACK: {{.*}}(32,8) : error(clang): extra tokens at end of
> #endif directive
> > +// DEFAULT: {{.*}}:36:8: warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > +// MSVC2010: {{.*}}(36,7) : warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > +// MSVC: {{.*}}(36,8) : warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > +// VI: {{.*}} +36:8: warning: extra tokens at end of #endif directive
> [-Wextra-tokens]
> > +// MSVC_ORIG: {{.*}}(36) : warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > +// NO_COLUMN: {{.*}}:36: warning: extra tokens at end of #endif
> directive [-Wextra-tokens]
> > +// MSVC2010-FALLBACK: {{.*}}(36,7) : error(clang): extra tokens at end
> of #endif directive
> > +// MSVC-FALLBACK: {{.*}}(36,8) : error(clang): extra tokens at end of
> #endif directive
> > int x;
> >
>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140701/2f18d97c/attachment.html>
More information about the cfe-commits
mailing list