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

[Daniel D. Daugherty]

On Wed, 7 Sep 2022 20:09:05 GMT, Alex Menkov <amenkov at 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