On Sat, 10 Sep 2022 15:11:33 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
> I like the fact that the fix is small and I really like the new test. I only
> have minor comments and a couple of questions.
Thank you for the review
> Please run these changes thru Tier[456] since that's where JVM/TI tests run
> in different configs w/ different options.
In progress
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4333:
>
>> 4331:
>> 4332: if (scratch_class->get_cached_class_file() !=
>> the_class->get_cached_class_file()) {
>> 4333: // 1. the_class doesn't have a cache yet, scratch_class does have.
>
> typo: s/does have./does have a cache./
Fixed
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4337:
>
>> 4335: // are multiple concurrent RetransformClasses calls on different
>> threads.
>> 4336: // the_class and scratch_class have the same cached bytes, but
>> different buffers.
>> 4337: // In such cases we need to deallocate one of the buffer.
>
> typo: s/the buffer/the buffers/
fixed
> typo: s/has cached/have cached/
The comment is about processing RedefineClasses when the_class has cached bytes.
So it should be "has"
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4340:
>
>> 4338: // 3. RedefineClasses and the_class has cached bytes from previous
>> transformation.
>> 4339: // In the case we need to use class bytes from scratch_class.
>> 4340: if (the_class->get_cached_class_file() != 0) {
>
> s/!= 0/!= nullptr/
fixed.
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
> line 174:
>
>> 172: throw new RuntimeException("Redefine error (ver = " + ver +
>> ")");
>> 173: }
>> 174: // verity ClassFileLoadHook get expected class bytes
>
> typos: s/verity/verify/
> s/get/gets the/
fixed
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
> line 197:
>
>> 195: int testCase;
>> 196: try {
>> 197: testCase= Integer.valueOf(args[0]);
>
> nit: please add space before '='
fixed.
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
> line 215:
>
>> 213: redefine(1); // cached class bytes are not set
>> 214: retransform(2, 1); // sets cached class bytes to ver 1
>> 215: redefine(3); // resets cached class bytes
>
> Perhaps:
> ```// resets cached class bytes to nullptr```
done
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
> line 224:
>
>> 222: redefine(1); // cached class bytes are not set
>> 223: retransform(2, 1); // sets cached class bytes to ver 1
>> 224: redefine(3); // resets cached class bytes
>
> Perhaps:
> ```// resets cached class bytes to nullptr```
done
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
> line 232:
>
>> 230: test("Retransform-Redefine-Retransform-Retransform", () -> {
>> 231: retransform(1, 0); // sets cached class bytes to ver 0
>> (initially loaded)
>> 232: redefine(2); // resets cached class bytes
>
> Perhaps:
> ```// resets cached class bytes to nullptr```
done
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/libRedefineRetransform.cpp
> line 26:
>
>> 24: #include <jvmti.h>
>> 25: #include <jni.h>
>> 26: #include <string.h>
>
> Should be in alpha sort order?
done
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/libRedefineRetransform.cpp
> line 48:
>
>> 46: }
>> 47:
>> 48: class ClassFileLoadHookHelper {
>
> A short comment describing the purpose of the `ClassFileLoadHookHelper` would
> be helpful to folks that only have a high level understanding of
> RedefineClasses()
> and RetransformClasses().
>
> You did a very good job encapsulating support for a complicated sets of APIs
> into this helper.
Added short description
> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/libRedefineRetransform.cpp
> line 215:
>
>> 213: caps.can_redefine_classes = 1;
>> 214: caps.can_retransform_classes = 1;
>> 215: jvmti->AddCapabilities(&caps);
>
> Why no error check here?
Added
-------------
PR: https://git.openjdk.org/jdk/pull/10032
