RFR: 8235966: Process obsolete flags less aggressively

[David Holmes]

Getting back to this ...

Please see updated webrev at:

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

Apologies as I mistakenly overwrote the original instead of creating v3.

This version expands on the original proposal by uncommenting the 
warnings about obsolete/expired flags that have not been removed from 
globals*.hpp, so that we don't forget to do this work. However these 
warnings are only enabled from build 20. I used 20 as being approx 2/3 
through the release cycle - long enough that the work should have been 
performed by then, whilst still leaving time to perform the work before 
RDP2. Of course we can tweak this number if there are issues with that 
choice.

Thanks,
David

On 20/12/2019 8:20 am, David Holmes wrote:
> 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
>>>>> -----
>>>>>
>>>>
>>