On Fri, 15 Nov 2024 15:30:05 GMT, Coleen Phillimore <[email protected]> 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
