RFR: 8066821(S) Enhance command line processing to manage deprecating and obsoleting -XX command line arguments

[Derek White]

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