RFR 8226304: Obsolete the -XX:+FailOverToOldVerifier option

[Harold Seigel]

Hi Coleen,

I can make those changes before pushing the change.

Thanks, Harold

On 6/19/2019 10:38 AM, coleen.phillimore at oracle.com wrote:
>
>
> 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
>>>>
>>>
>