RFR: 8066821(S) Enhance command line processing to manage deprecating and obsoleting -XX command line arguments
David Holmes
david.holmes at oracle.com
Mon Aug 10 05:53:32 UTC 2015
Hi Derek,
I don't have any further comments.
Thanks,
David
On 7/08/2015 12:46 PM, Derek White wrote:
> Another RFR.
>
> I've updated based on David and Kim's last comments.
>
> http://cr.openjdk.java.net/~drwhite/8066821/webrev.07/
> https://bugs.openjdk.java.net/browse/JDK-8066821
>
> I also have a webrev of webrev.06 vs. webrev.07:
> http://cr.openjdk.java.net/~drwhite/8066821/webrev.6.v.7/
>
> [This webrev is confused about CommandLineOptionTest.java. I
> double-checked with a recursive diff and the only differences are in
> arguments.cpp and arguments.hpp.]
>
> Thanks for looking!
>
> - Derek
>
> On 7/23/15 12:09 PM, Derek White wrote:
>> On 7/20/15 10:03 PM, David Holmes wrote:
>>> On 21/07/2015 3:02 AM, Derek White wrote:
>>>> Hi David,
>>>>
>>>> Thanks for looking this over.
>>>>
>>>> On 7/20/15 5:29 AM, David Holmes wrote:
>>>>> Hi Derek,
>>>>>
>>>>> Sorry but I'm finding this a bit confused and inconsistent. Comments
>>>>> below.
>>>>>
>>>>> On 18/07/2015 3:30 AM, Derek White wrote:
>>>>>> Request for review:
>>>>>>
>>>>>> [This updated webrev is being sent to wider audience, and has been
>>>>>> merged with Gerard's option constraints check-in. Also factored out
>>>>>> -XX:+AggressiveHeap processing into it's own chapter. I mean function
>>>>>> :-)]
>>>>>>
>>>>>> http://cr.openjdk.java.net/~drwhite/8066821/webrev.06/
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8066821
>>>>>
>>>>> argument.cpp:
>>>>>
>>>>> 258 * been not scheduled).
>>>>>
>>>>> -> not been scheduled
>>>>
>>>> OK.
>>>>> 260 * OBSOLETE: An option may be removed (and deleted from
>>>>> globals.hpp), but still be accepted on the command
>>>>> 261 * line. A warning is printed to let the user know
>>>>> that support may be removed in the future.
>>>>>
>>>>>
>>>>> Isn't this the stronger case that support has already been removed
>>>>> (the option does nothing) and it will be removed at the next major
>>>>> release. At the start of a major release we should be able to delete
>>>>> all entries from the obsolete table - else it wasn't obsolete but
>>>>> deprecated.
>>>>>
>>>>> Not sure "obsolete" is the right term here - obsolete things still
>>>>> function. For us an obsolete option does nothing (it exists only in
>>>>> the obsolete table).
>>>> It's not a great name, but that what the previous code called it.
>>>> It's a
>>>> "I'll pretend you didn't say that" option, like when a teenager
>>>> hears an
>>>> adult use out-of-date slang ("groovy!"). How about a "condescending"
>>>> option :-)
>>>
>>> Name aside you didn't comment on my main comment here: an obsolete
>>> option has already been removed from globals etc and does nothing.
>>
>> Ahh, OK. I must have been confusing in tense. I'll rewrite to be more
>> direct.
>>>>>
>>>>> 276 * Tests: Aliases are tested in VMAliasOptions.java.
>>>>> 277 * Deprecated options are tested in
>>>>> VMDeprecatedOptions.java.
>>>>> 278 * Obsolete options are tested in various files.
>>>>>
>>>>> We don't normally document this kind of thing in the source.
>>>>
>>>> I'm trying to head off unnecessary one-off test files for each alias
>>>> and
>>>> deprecated option. For example TestNoParNew.java and
>>>> TestDefaultMaxRAMFraction.java. And I think that there should be one
>>>> test file for obsolete options also (perhaps expanding
>>>> ObsoleteFlagErrorMessage.java), instead of a bunch of separate test
>>>> files, but that is not in this webrev.
>>>
>>> Sounds fine but again we don't normally document test strategies in
>>> the source code.
>> Normally I'd agree but I'm trying to change current practice of tests
>> popping up on the corpses of dead or dying options like mushrooms in a
>> forest. I'm doubly adamant if I can add that last sentence to the
>> comment :-)
>>>
>>>>> 281 // Obsolete or deprecated -XX flag.
>>>>>
>>>>> I can tell this is going to get confusing already.
>>>>>
>>>>> 284 JDK_Version obsoleted_in; // When the warning started (obsolete
>>>>> or deprecated).
>>>>>
>>>>> But there is a difference: deprecated == warn but still process;
>>>>> obsolete == warn and ignore.
>>>> Yes, but the SpecialFlag type is concerned with the common aspect of
>>>> warning. The "ignore" or "process" aspects are handled by the different
>>>> functions that look up the obsolete_jvm_flags and deprecated_jvm_flags
>>>> arrays.
>>>
>>> So that variable should really be obsoleted_or_deprecated_in ?
>>
>> OK, I see now. It was right in front of me. Maybe "warning_started_in"
>> is simpler.
>>>
>>>>>
>>>>> 288 // When a flag is eliminated, it can be added to this list in
>>>>> order to
>>>>> 289 // continue accepting this flag on the command-line, while
>>>>> issuing a warning
>>>>> 290 // and ignoring the value. Once the JDK version reaches the
>>>>> 'accept_until'
>>>>> 291 // limit, we flatly refuse to admit the existence of the flag.
>>>>> 292 static SpecialFlag const obsolete_jvm_flags[] = {
>>>>>
>>>>> When a flag is eliminated it is gone from this table. As soon as the
>>>>> accept_until version is the current version we wipe the table of all
>>>>> such entries. This should be one of the first things done in a new
>>>>> release.
>>>>
>>>> I completely agree that this is a great plan. But until this April we
>>>> still had obsolete flags listed for JDK 5! The obsolete_jvm_flags table
>>>> did the right thing until the process caught up. In any case, this
>>>> webrev doesn't really change the behavior of the obsolete_jvm_flags
>>>> table.
>>>
>>> But what you are seeing before April is the result of hotspot express
>>> (at least in a large part). Now that we don't have to support that we
>>> don't have legacy lists to maintain. The code could even be changed
>>> upon detecting that current JDK version == "until" version to report
>>> "Hey you forgot to update the table!" ;-)
>> OK, that history makes more sense.
>>> My point is that the comments should reflect how we intend to use
>>> these, not give the impression that keeping eliminated options in the
>>> table is the expected thing to do.
>>
>> OK, I'll update the comments both here and up above to describe the
>> presumably common "deprecated"->"obsolete" lifecycle.
>>>>>
>>>>> 320 // When a flag is deprecated, it can be added to this list in
>>>>> order to issuing a warning when the flag is used.
>>>>> 321 // Once the JDK version reaches the 'accept_until' limit, we
>>>>> flatly refuse to admit the existence of the flag.
>>>>> 322 static SpecialFlag const deprecated_jvm_flags[] = {
>>>>>
>>>>> A deprecated flag should be obsoleted before it reaches the
>>>>> accept_until limit.
>>>> That's a policy that's under discussion on hotspot-runtime-dev. There
>>>> are certainly option lifecycles that have been approved by our internal
>>>> process that don't follow this proposed policy. The mechanism in this
>>>> webrev was concerned about supporting the plethora of current
>>>> lifecycles, for better or worse.
>>>
>>> But again comments should reflect expected usage - if we reach the
>>> "until" version of a deprecated option that wasn't obsoleted then
>>> something has probably gone wrong.
>>
>> OK.
>>>>> Which suggests that when we start a new version we wipe the obsoleted
>>>>> table and move the deprecated options into it.
>>>> I can add documentation to describe this case.
>>>>
>>>> If we decide that we'll always treat a deprecated or aliased option as
>>>> obsolete for one extra release, then it might make sense to have a
>>>> combined option lifecycle table. But for now I like the fact that
>>>> options in deprecated_jvm_flags should always have a real variable
>>>> defined in globals.hpp (etc), and options in obsolete_jvm_flags should
>>>> never have a global variable.
>>>
>>> Yes I like that too.
>>>>
>>>>> 776 case 1: {
>>>>> 777 if (warn) {
>>>>> 778 char version[256];
>>>>> 779 since.to_string(version, sizeof(version));
>>>>> 780 if (real_name != arg) {
>>>>> 781 warning("Option %s was deprecated in version %s and
>>>>> will likely be removed in a future release. Use option %s instead.",
>>>>> 782 arg, version, real_name);
>>>>> 783 } else {
>>>>> 784 warning("Option %s was deprecated in version %s and
>>>>> will likely be removed in a future release.",
>>>>> 785 arg, version);
>>>>> 786 }
>>>>>
>>>>> This isn't distinguishing between deprecated and obsoleted ???
>>>>
>>>> Yes, handle_aliases_and_deprecation() doesn't handle obsolete options
>>>> (or it would have had a longer name :-) Maybe it should handle that
>>>> case, but it would have complicated the control flow in the caller. I
>>>> have another proposed change in the works that simplifies the caller
>>>> quite a bit that would make the refactoring simpler.
>>>
>>> I need to think more on that. It is hard to see the overall control
>>> flow for argument processing.
>>
>> Yes it is hard to see. Currently options are parsed into tokens in a
>> couple of places. It's roughly either:
>> [+][-]arg
>> or:
>> arg[:]=[value]
>>
>> But not all of the code handles ":=". Furthermore option processing
>> figures out the type of the argument by repeatedly trying to scanf
>> "value" to see if it looks like floating point, or an integer, or
>> maybe just text. I think we should first parsing out the arg name, get
>> the Flag, and ask the Flag what type it is. This would make it much
>> easier to handle aliases, deprecation, and obsoleting options in one
>> place.
>>
>> I have a workspace that does this but I suspect there could be slight
>> differences in how errors would be reported.
>>>
>>> Thanks,
>>> David
>> Thanks for looking at this again. It's heading in the right direction!
>> - Derek
>>
>
More information about the hotspot-dev
mailing list