RFR: 8235966: Process obsolete flags less aggressively

Wed Dec 18 17:37:29 UTC 2019

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
>>> -----
>>>
>>