RFR: 8235966: Process obsolete flags less aggressively

[David Holmes]

Hi Dan,

Thanks for taking a look. Updated webrev:

http://cr.openjdk.java.net/~dholmes/8235966/webrev.v2/

Discussion below.

On 18/12/2019 1:47 am, Daniel D. Daugherty wrote:
> On 12/16/19 12:16 AM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8235966
>> webrev: http://cr.openjdk.java.net/~dholmes/8235966/webrev/
> 
> src/hotspot/share/runtime/arguments.cpp
>      L745:       // if flag has become obsolete it should not have a 
> "globals" flag defined anymore.
>      L746:       if (!version_less_than(JDK_Version::current(), 
> flag.obsolete_in)) {
>      L747:         if (JVMFlag::find_declared_flag(flag.name) != NULL) {
>      L748:           // Temporarily disable the warning: 8196739
>      L749:           // warning("Global variable for obsolete special 
> flag entry \"%s\" should be removed", flag.name);
>      L750:         }
>      L751:       }
>          It seems like we've been down a similar road before:
> 
>          JDK-8196739 Disable obsolete/expired VM flag transitional warnings
>          https://bugs.openjdk.java.net/browse/JDK-8196739
> 
>          This one may ring a bell... Fixed by dholmes in jdk11-b01... :-)
> 
>          And this followup sub-task to re-enable that warning:
> 
>          JDK-8196741 Re-enable obsolete/expired VM flag transitional 
> warnings
>          https://bugs.openjdk.java.net/browse/JDK-8196741
> 
>          was closed as "Won't fix" on 2019.08.02.
> 
>          So the obvious questions:
> 
>          - Why is the new warning less problematic to tests that don't
>            tolerate unexpected output?

Two different situations. The commented out warning happens 
unconditionally when you run the VM and it finds any flag marked 
obsolete that hasn't been removed. Hence every single test will 
encounter this warning.

The situation I am modifying is when a test uses a flag that is marked 
for obsoletion. In the majority of cases the flag is already deprecated 
and so already issuing a deprecation warning that the test has to 
handle. Without my change there would still be an obsoletion warning, so 
this test is in for a warning no matter what.

Also note that for hotspot at least we have strived to make tests 
tolerate unexpected output. The reason JDK-8196741 was closed as "won't 
fix" was because other areas wouldn't commit to doing that.

>          - If you move forward with this fix, then I think think code
>            block needs to be removed or modified or am I missing something?

I've rewritten the comment at the head of verify_special_jvm_flags to 
explain why we can't issue a warning, and have deleted the block.

>          There's a similar commented out check on L757-L765, but that one
>          is for an expired flag... You might want to adjust/delete it also?

Deleted.

>      L753:         warning("Special flag entry \"%s\" must be explicitly 
> obsoleted before expired.", flag.name);
>      L754:         success = false;
>          nit - s/before expired/before being expired/
>          Update: I now see that "style" is in several places in this
>              function. I'm not sure what to think here... it grates,
>              but I can live with it.
> 
>          nit - L75[34] indented too much by two spaces.

Fixed.

>      L962:           return real_name;
>          nit - indented too much by two spaces.

Fixed.

> 
> Trying to understand the modified logic in argument processing is
> making my head spin...

Mine too. It took a few attempts to put the logic in the right place and 
make adjustments so that it all works as expected for a correctly 
specified flag and an erroneous one.

> - You've added a call to is_obsolete_flag() in
>    handle_aliases_and_deprecation() and is_obsolete_flag()
>    is where the new warning is output:
> 
>      warning("Temporarily processing option %s; support is scheduled for 
> removal in %s"
> 
>    handle_aliases_and_deprecation() is called from six different places,
>    but the call sites are different based on the argument pattern so I
>    have (mostly) convinced myself that there should not be any duplicate
>    warning lines.

Right - handle_aliases_and_deprecation is only called for a 
syntactically correct flag based on those patterns. It normally filters 
out obsoleted/expired flags and lets them fall through to later error 
processing (in process_argument after parse_arg returns false). That 
error processing is where the normal obsoletion check is performed. So I 
had to not filter the flag in handle_aliases_and_deprecation in this 
case, but still produce the warning for a malformed flag. E.g.

java -XX:+UseParallelOldGC -version
Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing option 
UseParallelOldGC; support is scheduled for removal in 15.0
java version "15-internal" 2020-09-15

java -XX:UseParallelOldGC -version
Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing option 
UseParallelOldGC; support is scheduled for removal in 15.0
Missing +/- setting for VM option 'UseParallelOldGC'
Error: Could not create the Java Virtual Machine.

> So I now understand the new logic that allows an obsoleted option
> to be specified with a warning as long as the option still exists.
> I'm good with the technical change, but...
> 
> I'm worried about tests that don't tolerate the new warning mesg,
> i.e., why wouldn't this become an issue again:
> 
> JDK-8196739 Disable obsolete/expired VM flag transitional warnings

Explained above.

Thanks,
David

> Dan
> 
> 
>>
>> When a flag is marked as obsolete in the special-flags table we will 
>> ignore it and issue a warning that it is being ignored, as soon as we 
>> bump the version of the JDK. That means that any tests still using the 
>> obsolete flag may start to fail, leading to a surge of test failures 
>> at the start of a release cycle. For example for JDK 15 we have a 
>> whole bunch of JFR tests that fail because they still try to work with 
>> UseParallelOldGC. In another case 
>> runtime/cds/appcds/FieldLayoutFlags.java passes only be accident.
>>
>> When a flag is marked as obsolete for a release, all code involving 
>> that flag (including tests that use it) must be updated within that 
>> release and the flag itself removed. Whilst this is typically 
>> scheduled early in a release cycle it isn't reasonable to expect it to 
>> all occur within the first couple of days of the release cycle, nor do 
>> we want to have to ProblemList a bunch of tests when they start failing.
>>
>> What I propose is to instead allow an obsolete flag to continue to be 
>> processed as long as that code removal has not actually occurred - 
>> with an adjusted warning. The change I propose:
>>
>> - only treats an obsolete flag as obsolete if the flag cannot be found
>> - added a new flag verification rule that disallows obsoletion in an 
>> undefined version, but expiration in a specific version i.e. we must 
>> always explicitly obsolete a flag before we expire it.
>>
>> The only downside here is that if we actually forget to file an issue 
>> for the actual obsoletion work we may not notice via testing. Of 
>> course whenever a change is made to the flags table to add an entry 
>> then the issue to do the obsoletion should be filed at the same time.
>>
>> Thanks,
>> David
>> -----
>>
>