r211420 - Driver: enhance MSC version compatibility
Saleem Abdulrasool
abdulras at fb.com
Mon Jun 23 09:27:46 PDT 2014
On Jun 23, 2014, at 6:38 AM, Alp Toker <alp at nuanti.com> wrote:
>
> On 23/06/2014 15:02, Aaron Ballman wrote:
>> On Sun, Jun 22, 2014 at 10:09 PM, Alp Toker <alp at nuanti.com> wrote:
>>> On 23/06/2014 04:30, Saleem Abdulrasool wrote:
>>>> On Sun, Jun 22, 2014 at 4:15 PM, Alp Toker <alp at nuanti.com
>>>> <mailto: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> <mailto: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.
>>>
>>> Accepting a dotted version is fine. Overloading the existing -fmcs-version=
>>> option is the problem..
>> Given that this is meant to be backwards compatible, I'm not certain I
>> understand what the problem actually is. If the option isn't backwards
>> compatible, that's simply a bug we should fix.
>>
>>>>
>>>> * 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.
>>>
>>> Nope, definitely specific to this change.
>> Can you elaborate?
>>
>>>
>>>> * 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.
>>>
>>> External tools need to parse and grok driver arguments. Supporting two
>>> syntaxes in a single option requires custom handling to be developed in each
>>> tool -- there are very few other options like yours in the clang driver.
>>> Where they exist, it's done out of necessity to support GCC bugs.
>>>
>>> Your feature could easily be implemented as a second flag at no cost, and
>>> that seems to be the obvious way to go. Is there a problem with actually
>>> doing that?
>> Then users (whose experience I care about *far* more than external
>> tools vendors) have two options which serve the same purpose, and
>> creates some confusion as to which one should be used, what situations
>> they should use them in, and why. Having a single option presents a
>> better user experience, IMO.
>
> This is interesting. To me, it's as clear as day that accepting two incompatible version formats in a single option flag is bad for users and tools. It's also unprecedented -- we've not done this before with any driver option. There are many options that have multiple forms for different inputs syntax.
>
> Why is this flag so special that we need to break all the rules and introduce command-line parsing disambiguations?
>
> It's not at all obvious to a user that '17.0' or '17.0.0' defines macros as version 17 while '17' defines macros as version 0.0.0.17. Predicating on presence of a '.' is going to look like a bug to any user who encounters it.
This seems truly insincere. Do you honestly expect developers to claim, version 17 is 0.0.17 (and before someone attempts to claim that Enlightenment did this, they said E17, aka 0.17, not version 17)? The dot delimited versioning scheme is nearly universally used as major[.minor[.patch[.release]]]]. I think expecting that the user specifies it as such is perfectly reasonable.
@aaron, I think I agree with you that having a single flag is much nicer from a user’s PoV.
> Another point, clang 3.4 and earlier driver will actively diagnose the new flag and let the user know:
>
> New option flag with the 3.4 driver:
>
> $ clang -fmsc-full-version=15.00.20706.01 -###
> clang version 3.5.0
> Target: x86_64-apple-darwin13.2.0
> Thread model: posix
>
> Whereas the overloaded option flag produces an error to let the user know:
>
> $ clang -fmsc-version=15.00.20706.01 -###
> clang-3.4: error: unknown argument: '-fmsc-full-version=15.00.20706.01'
> clang version 3.5.0
> Target: x86_64-apple-darwin13.2.0
> Thread model: posix
>
> And the former is just more work for configure scripts to detect whether the syntax is supported. They have to run a real compilation to determine if there's an error for no good reason at all.
>
> A flag accepting a specific and long version number like "15.00.20706.01" needs to have really clear semantics and documentation. The obvious way to achieve that is to have two flags -- one that accepts the legacy encoded version number, and a new one that accepts the user-friendly format.
>
> Also have we confirmed that the Visual Studio variables actually provide a version number in this format (I think so, but we should dogfood it). Right now we have to pre-process the value on the LLVM side before feeding it into clang and that's the primary consumer of this flag.
>
> Alp.
>
>>
>>>
>>>
>>>> * 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.
>>>
>>> I don't see how you could accommodate that without adding more
>>> disambiguations, checking for '.' characters or testing the input length, or
>>> perhaps detecting if it's a very large or small number.
>> We already check for the '.' character in this patch... extending that
>> hardly seems problematic?
>>
>>> The base case where the input is integral will always conflict -- you have
>>> no way to guess which one the user intended.
>>>
>>> The quirks are already problematic and adding more isn't the solution here:
>>> This could easily be made into two distinct options, and we'll then be able
>>> to clearly document the newly-added option so users know straight away if
>>> it's supported in their version of clang.
>> I'm weakly opposed to adding a second flag here, because I'm not
>> convinced one is required, or desirable.
>>
>> ~Aaron
>
> --
> http://www.nuanti.com
> the browser experts
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
Saleem Abdulrasool
abdulras (at) fb (dot) com
More information about the cfe-commits
mailing list