On Mon, 9 Dec 2024 02:08:58 GMT, David Holmes <dhol...@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
