On Fri, 22 Aug 2025 15:06:46 GMT, Chen Liang <li...@openjdk.org> wrote:
>> For early eval; test by changing the ClassReader max accepted version of >> test ASM to 24 instead of 25 > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 11 additional commits since > the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/asm-test-upgrade > - Merge branch 'fix/asm-test-upgrade' of github.com:liachmodded/jdk into > fix/asm-test-upgrade > - Update test/hotspot/jtreg/compiler/calls/common/InvokeDynamicPatcher.java > > Co-authored-by: Andrew Haley <aph-o...@littlepinkcloud.com> > - Variable name improvements > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/asm-test-upgrade > - Move other tier 4,5, etc tests to not use ClassReader > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/asm-test-upgrade > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/asm-test-upgrade > - Use classfile api instead of javac setting version > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/asm-test-upgrade > - ... and 1 more: https://git.openjdk.org/jdk/compare/e0d5ae0b...a659f538 I looked at the diff with whitespace off and looked at all change. They seem reasonable to me. The bytecode changes seems to be 1-1 matching with the old code. I just have a few style nits. Since the changes are subtle, I think before integration, you should do a final run up to tier 6 just to be safe. test/hotspot/jtreg/compiler/jvmci/common/CTVMUtilities.java line 80: > 78: throw new Error("TEST BUG: cannot read " + binaryName, e); > 79: } > 80: ClassModel classModel = ClassFile.of().parse(fileBytes); Can you put these in a separate function, like `ClassModel getClassModel(Class<?> class)`? test/hotspot/jtreg/compiler/jvmci/common/CTVMUtilities.java line 93: > 91: for (var methodModel : classModel.methods()) { > 92: if (methodModel.methodName().equalsString(methodName) > 93: && methodModel.methodType().isMethodType(methodType)) > { For readability, I think we should put the above in a separate function, and then: var methodModel = findMethodInClass(classModel, m); Then we can potentially use findMethodInClass() in other test cases when we have a need to do so. test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/MissedStackMapFrames/MissedStackMapFrames.java line 64: > 62: var foundStackMapTable = method.code().flatMap(code -> > code.findAttribute(Attributes.stackMapTable())); > 63: if (foundStackMapTable.isPresent()) { > 64: count += foundStackMapTable.get().entries().size(); The old logging should be preserved: log(" method " + name + " - " + methodFrames + " frames"); test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineAnnotations.java line 104: > 102: public void atEnd(ClassBuilder builder) { > 103: // Re-add dummy fields > 104: dummyFields.forEach(builder); Comment: `// Re-add dummy fields at the end of the class` ------------- Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25124#pullrequestreview-3182050159 PR Review Comment: https://git.openjdk.org/jdk/pull/25124#discussion_r2319959194 PR Review Comment: https://git.openjdk.org/jdk/pull/25124#discussion_r2319956360 PR Review Comment: https://git.openjdk.org/jdk/pull/25124#discussion_r2319921409 PR Review Comment: https://git.openjdk.org/jdk/pull/25124#discussion_r2319969688
