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

Derek White derek.white at oracle.com
Fri Aug 7 02:46:56 UTC 2015

Another RFR.

I've updated based on David and Kim's last comments.


I also have a webrev of webrev.06 vs. webrev.07:

[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