RFR 8226304: Obsolete the -XX:+FailOverToOldVerifier option

[coleen.phillimore at oracle.com]

On 6/19/19 9:02 AM, Harold Seigel wrote:
> Hi Coleen,
>
> Thanks for looking at this.
>
> The logic concerning the split verifier not setting pending exception 
> is not affected by this change.  It existed before this change.
>
> Do you think this looks better?  Should other compound 'if' statements 
> in hotspot be broken up into assignments to bool variables?

This looks better, because then you can associate the interesting 
comment about DumpSharedSpaces with this boolean and not the complicated 
if statement.  In answer to your question about other compound 'if' 
statements, my answer is that it depends on how surprising the logic is 
in the clauses, as this is.

I realize the HAS_PENDING_EXCEPTION was pre-existing, even though it's 
unexpected.  Can you add a short comment like:
>
>         bool can_failover = !DumpSharedSpaces &&
>            klass->major_version() < NOFAILOVER_MAJOR_VERSION;
>         if (can_failover && !HAS_PENDING_EXCEPTION && // split 
> verifier doesn't set PENDING_EXCEPTION for failure
>             (exception_name == vmSymbols::java_lang_VerifyError() ||
>              exception_name == 
> vmSymbols::java_lang_ClassFormatError())) {
>

I don't need to see another version of this change if you choose to make 
these edits.

Thanks,
Coleen

> Thanks, Harold
>
>
> On 6/19/2019 8:49 AM, coleen.phillimore at oracle.com wrote:
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8226304/webrev/src/hotspot/share/classfile/verifier.cpp.frames.html 
>>
>>
>> 180
>> 181 // If DumpSharedSpaces is set then don't fall back to the old 
>> verifier on
>> 182 // verification failure. If a class fails verification with the 
>> split verifier,
>> 183 // it might fail the CDS runtime verifier constraint check. In 
>> that case, we
>> 184 // don't want to share the class. We only archive classes that 
>> pass the split
>> 185 // verifier.
>> 186 if (klass->major_version() < NOFAILOVER_MAJOR_VERSION &&
>> 187 !DumpSharedSpaces && !HAS_PENDING_EXCEPTION &&
>>  188         (exception_name == vmSymbols::java_lang_VerifyError() ||
>>  189          exception_name == 
>> vmSymbols::java_lang_ClassFormatError())) {
>>
>>
>> This logic is torturous.  Does the split verifier not set 
>> pending_exception, but sets the exception_name ?  Weird.
>>
>> I kind of prefer the can_failover boolean better, adding 
>> !DumpSharedSpaces to that.
>>
>> Coleen
>>
>> On 6/18/19 3:06 PM, Harold Seigel wrote:
>>> Hi,
>>>
>>> Please review this JDK-14 fix to obsolete the 
>>> -XX+FailOverToOldVerifier option.
>>>
>>> Open Webrev: 
>>> http://cr.openjdk.java.net/~hseigel/bug_8226304/webrev/index.html
>>>
>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8226304
>>>
>>> The fix was tested by running with option FailOverToOldVerifier and 
>>> checking the output:
>>>
>>>     > java -XX:+FailOverToOldVerifier -version
>>>    Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
>>>    FailOverToOldVerifier; support was removed in 14.0
>>>    java version "14-internal" 2020-03-17
>>>                ...
>>>
>>> It was regression tested by running Mach5 tiers 1 and 2 tests and 
>>> builds on Linux-x64, Solaris, Windows, and Mac OS X, by running 
>>> Mach5 tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on 
>>> Linux-x64.
>>>
>>> Thanks, Harold
>>>
>>