r211420 - Driver: enhance MSC version compatibility
Alp Toker
alp at nuanti.com
Mon Jun 23 11:31:22 PDT 2014
On 23/06/2014 21:21, Aaron Ballman wrote:
> On Mon, Jun 23, 2014 at 2:18 PM, Alp Toker <alp at nuanti.com> wrote:
>> On 23/06/2014 20:48, Aaron Ballman wrote:
>>> On Mon, Jun 23, 2014 at 1:36 PM, Alp Toker <alp at nuanti.com> wrote:
>>>> On 23/06/2014 19:54, Aaron Ballman wrote:
>>>>> On Mon, Jun 23, 2014 at 12:50 PM, Alp Toker <alp at nuanti.com> wrote:
>>>>>> On 23/06/2014 19:27, Saleem Abdulrasool wrote:
>>>>>>> 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
>>>>>>
>>>>>> Hi Saleem,
>>>>>>
>>>>>> That's what your patch does right now, which is the bug I'm reporting.
>>>>>>
>>>>>> You missed the base case in your code so when the user specifies 17 it
>>>>>> turns
>>>>>> into 0.0.0.17 with your patch.
>>>>> I think we're all agreed that behavior is simply a bug that needs to
>>>>> be resolved. 17 should become 17.0.
>>>>
>>>> Yes, and major[.minor[.patch[.release]]]] will conflict with the
>>>> traditional
>>>> integer forms conflict in that case.
>>>>
>>>> I don't see any way to support both integral forms cleanly with a single
>>>> option.
>>> The backwards compatible approach is to look at the value which comes
>>> in. It could only encode a specific range of values (MMmm), which
>>> means for MM values, 99 was the max. I'm perfectly comfortable saying
>>> any value that comes in < 100 is simply a major version.
>>
>> You're suggesting this?
>>
>> if (version < 100)
>> version *= 100;
>>
>> It doesn't make sense to me.
>>
>> Why parse an integer driver argument completely differently depending on
>> whether it's "very big or very small" when there's no burden of
>> compatibility that forces us to push the two totally value kinds into a
>> single flag?
> As has been mentioned before, it puts the burden on the end user of
> the flags. They would wind up with two flags which do the exact same
> thing, but in different ways with slight incompatibilities, and no
> benefit to them. Why increase the number of options needlessly when we
> can support this with a single option in a backwards compatible
> manner?
Hi Aaron,
I provided an explanation of why another option should be added here
just below where you snipped my mail:
> On Mon, Jun 23, 2014 at 2:18 PM, Alp Toker <alp at nuanti.com> wrote:
>> There's a really elegant way to do this that's also very neatly
>> documented, versioned and more discoverable, while requiring no
>> disambiguation:
>>
>> -fmsc-version=MMmmbbbbb
>> -fmsc-full-version=major[.minor[.patch[.release]]]]
>>
>> What's more, those types are both represented by standard masks in at
>> least XCode and MSVC and probably other tools too. That's really cool
>> to me and strongly indicates we want two option flags here.
>> It makes sense to add a new option because it's a different input
>> type, and the disambiguation is quirky.
Apart from that there's also the point of canonicalisation. Any tool
that models driver options (e.g. using compilation database support in
libclang, or ccache which has its own algorithms) would have to
reimplement the if (version < 100) version *= rule.
As for end users, "fmsc-full-version" is nice and easy to search on the
web or grep in the documentation, whereas silently adding quirks isn't.
Alp.
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list