RFR: 8235966: Process obsolete flags less aggressively

[Daniel D. Daugherty]

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

         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?

     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.

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


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

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

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

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