RFR: 8344922: Redefinition verifies the new klass when verification is disabled

[Coleen Phillimore]

On Mon, 9 Dec 2024 02:08:58 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This change only verifies redefined classes if Verification is enabled.  BytecodeVerificationRemote will be false for verification turned off.  If someone turns it off but BytecodeVerificationLocal on (which is non-sensical), the argument processing code will fix that up.  So all this needs to do is check for BytecodeVerifificationRemote for -Xverify:none (which is a deprecated option).
>> 
>> 
>>   // Treat the odd case where local verification is enabled but remote
>>   // verification is not as if both were enabled.
>>   if (BytecodeVerificationLocal && !BytecodeVerificationRemote) {
>>     log_info(verification)("Turning on remote verification because local verification is on");
>>     FLAG_SET_DEFAULT(BytecodeVerificationRemote, true);
>>   }
>> 
>> 
>> Tested with runtime/verifier, jck vm and tier1-4 (in progress), and the new test case.
>
> src/hotspot/share/classfile/verifier.cpp line 137:
> 
>> 135:   return (should_verify_class &&
>> 136:     // Override parameter (return false) if -Xverify:none
>> 137:     BytecodeVerificationRemote &&
> 
> Sorry but I don't understand why the correct value for `should_verify` doesn't filter through from `Verifier::should_verify_for`??? Seems to me that is the only place where we should explicitly check the verification flags.

I could check it in redefinition instead, here:

https://github.com/coleenp/jdk/blob/master/src/hotspot/share/prims/jvmtiRedefineClasses.cpp#L1461
and
https://github.com/coleenp/jdk/blob/master/src/hotspot/share/prims/jvmtiRedefineClasses.cpp#L1493

Or have another call should_verify_for_redefinition() which verifies things by default true even if from the bootclass.  Whichever you prefer.  The point is that we do want to verify bytecodes given during redefinition even for the bootclass path because they could be wrong and mess up everything.  Unless, one runs with -Xverify:none in which case you don't want to verify.  Although I said in the bug, this seems like a bad idea and there should be no reason to do this.  So, also maybe that can be the resolution.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22617#discussion_r1875981587