RFR: 8330606: Redefinition doesn't but should verify the new klass [v3]

[David Holmes]

On Fri, 15 Nov 2024 15:30:05 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change adds a couple of comments, removes some ancient bootstrapping code, and adds an old test that we call the verifier for redefining a class, even one in the bootclass path.
>> 
>> The fix for always verifying redefined classes was actually pushed with JDK-8341094, where the verifier code respected the parameter "should_verify_class".  By default this parameter follows the -Xverify setting.  For redefinition, this is passed as true.   The rest of the fix removes the special bootstrap loader cases that may have failed early on in the JDK development with -Xverify:all but now no longer do.  If someone tries to redefine these classes, they should also do the verification on the redefined bytecodes.
>> 
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reduce test, fix bug in verifier, move and add comments to is_eligible_for_verification.

Thanks for explaining how the main fix was done elsewhere.

A couple of comments below but changes seem okay.

Thanks

src/hotspot/share/oops/method.cpp line 339:

> 337: int Method::validate_bci(int bci) const {
> 338:   // Called from the verifier, and should return -1 if not valid.
> 339:   return ((is_native() && bci == 0) || (!is_native() && 0 <= bci && bci < code_size())) ? bci : -1;

This expansion seems more correct, but it also seems unrelated to the main changes.

test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineVerifyError.java line 96:

> 94:             throw new RuntimeException("This should throw VerifyError");
> 95:         } catch (VerifyError e) {
> 96:             // JVMTI recreates the VerifyError so the verification message is lost.

I'm not clear why JVMTI would mess with the `VerifyError` that the verifier should have raised, and which explains the reason why verification failed. Not that your changes impact that.

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22116#pullrequestreview-2447281814
PR Review Comment: https://git.openjdk.org/jdk/pull/22116#discussion_r1849463768
PR Review Comment: https://git.openjdk.org/jdk/pull/22116#discussion_r1849460625