On Wed, 7 Sep 2022 20:09:05 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> The problem is RedefineClasses does not update cached_class_bytes, so
>> subsequent RetransformClasses gets obsolete class bytes (this are testcases
>> 3-6 from the new test)
>>
>> cached_class_bytes are set when an agent instruments the class from
>> ClassFileLoadHook.
>> After successful RedefineClasses it should be reset.
>> The fix updates ClassFileLoadHook caller to not use old cached_class_bytes
>> with RedefineClasses (if some agent instruments the class, new
>> cached_class_bytes are allocated for scratch_class) and updates
>> cached_class_bytes after successful RedefineClasses or RetransformClasses.
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> updated test. comments, code reorg
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.
Please run these changes thru Tier[456] since that's where JVM/TI
tests run in different configs w/ different options.
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./
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/
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4338:
> 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.
> 4338: // 3. RedefineClasses and the_class has cached bytes from previous
> transformation.
typo: s/has cached/have cached/
typo: s/from previous/from a previous/
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/
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/
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 '='
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```
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```
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```
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
line 240:
> 238: case 5:
> 239: test("Redefine-Retransform-Redefine-Retransform with CFLH",
> () -> {
> 240: redefine(1, 5); // CFLH sets cached class bytes to
> ver 1
I'm having trouble understanding why the CFLH version is '5' here.
Update: I _think_ this is just to have the CFLH return a different version
of the class bytes before the RedefineClasses() call does its work. I
don't understand why you want to do this...
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
line 242:
> 240: redefine(1, 5); // CFLH sets cached class bytes to
> ver 1
> 241: retransform(2, 1); // uses existing cache
> 242: redefine(3, 6); // resets cached class bytes,
I'm having trouble understanding why the CFLH version is '6' here.
Update: I _think_ this is just to have the CFLH return a different version
of the class bytes before the RedefineClasses() call does its work. I
don't understand why you want to do this...
Perhaps:
```// resets cached class bytes to nullptr,```
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java
line 251:
> 249: test("Retransform-Redefine-Retransform-Retransform with
> CFLH", () -> {
> 250: retransform(1, 0); // sets cached class bytes to ver 0
> (initially loaded)
> 251: redefine(2, 5); // resets cached class bytes,
I'm having trouble understanding why the CFLH version is '5' here.
Update: I _think_ this is just to have the CFLH return a different version
of the class bytes before the RedefineClasses() call does its work. I
don't understand why you want to do this...
Perhaps
```// resets cached class bytes to nullptr,```
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?
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.
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?
-------------
Changes requested by dcubed (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10032
