RFR: 8235966: Process obsolete flags less aggressively

[David Holmes]

Thanks Dan.

FTR I've updated the bug report with an extension to this proposal, 
which is to add back the flag table validation checks to use via a gtest 
that we only enable after a certain build in the release cycle (it 
always passes before then). That way we avoid the problems I've outlined 
with the initial version bump but also have a safety net in place to 
ensure we don't forget to actually obsolete/expire flags.

Cheers,
David

On 19/12/2019 3:37 am, Daniel D. Daugherty wrote:
> Hi David,
> 
> On 12/17/19 5:03 PM, David Holmes wrote:
>> Hi Dan,
>>
>> Thanks for taking a look. Updated webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/8235966/webrev.v2/
> 
> src/hotspot/share/runtime/arguments.cpp
>      I like the updates to header comment for verify_special_jvm_flags().
> 
> Thumbs up.
> 
> 
>>
>> Discussion below.
> 
> Replies 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.
> 
> Ouch on such verbosity.
> 
> 
>> 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.
> 
> Good that your change only comes into play when the flag is used.
> 
> 
>> 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.
> 
> Yup. Got it.
> 
> 
>>
>>>          - 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.
> 
> Thanks for deleting the stale code.
> 
> 
>>
>>>          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.
> 
> Thanks.
> 
> 
>>
>>>      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.
> 
> I keep trying to convince myself that we're improving this flag and
> options code with each release... :-)
> 
> 
>>
>>> - 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.
> 
> Thanks for the example. That helps a lot.
> 
> 
>>
>>> 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.
> 
> Yup and thanks.
> 
> Dan
> 
> 
>>
>> 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
>>>> -----
>>>>
>>>
>