RFR: 7124710: interleaved RedefineClasses() and RetransformClasses() calls may have a problem [v3]

[Alex Menkov]

On Sat, 10 Sep 2022 15:11:33 GMT, Daniel D. Daugherty <dcubed at 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