RFR: 8235966: Process obsolete flags less aggressively
David Holmes
david.holmes at oracle.com
Sun Jan 19 21:46:53 UTC 2020
Hi Dan,
On 18/01/2020 1:37 am, Daniel D. Daugherty wrote:
> On 1/16/20 12:57 AM, David Holmes wrote:
>> Getting back to this ...
>
> You added this update to the bug report:
>
>> Update: after further discussion it has been proposed that we use the
>> build number as the trigger for a whitebox or gtest that performs the
>> currently disabled full verification of the flag table. So if a flag
>> has not be obsoleted or expired as it should by build N** then we fail
>> the gtest. This will complement the relaxing of the obsoletion check
>> at the start of the release cycle.
>
> I was expecting a new test in this latest webrev that would start failing
> at Build 20... let me see what the latest webrev says...
Mea culpa. When I started thinking about the test it was evident the
test logic would just involve the existing commented out warning logic.
So I thought lets just turn those warnings on at build 20 but ...
>>
>> Please see updated webrev at:
>>
>> http://cr.openjdk.java.net/~dholmes/8235966/webrev/
>
> src/hotspot/share/runtime/arguments.cpp
> No comments.
>
> So the changes are the same as the last round with the addition of
> enabling the following at B20:
>
> L755: warning("Global variable for obsolete special flag entry
> \"%s\" should be removed", flag.name);
> L769: warning("Global variable for expired flag entry \"%s\"
> should be removed", flag.name);
>
>
>> 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.
>
> Okay... but doesn't this mean that every test would issue these warnings
> as of B20 if we have not completed the work? So we have the potential of
> a raft (some unknown number) of test failures due to unexpected output in
> the form of these warning messages. And worse, these failures would be
> unrelated to actual issue... :-(
... as you note the end result is not really what we want - a single
clearly failing test.
> How about adding a diagnostic flag that enables these two warning
> messages (in addition to the B20 check). Add a single test that runs:
>
> java -XX:+UnlockDiagnosticVMOptions -XX:+FlagShouldNotBeDefinedCheck
> -version
>
> and when we hit B20, if there are still flags that haven't been removed,
> then the test will fail and we'll have one test that fails (X the number
> of configs that run the test).
I definitely do not want to add another VM flag :) but will look at how
to (re)run that logic as part of a gtest.
Thanks,
David
> Dan
>
>
>>
>> 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
>>>>>>> -----
>>>>>>>
>>>>>>
>>>>
>
More information about the hotspot-dev
mailing list