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

[Alex Menkov]

On Sat, 3 Sep 2022 09:47:05 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

> Also, I'm curious how did you verify that no regressions have been introduced?

Run all Redefine/Retransform Classes tests:
  test/jdk/java/lang/instrument
  test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
  test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
  test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses

> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java line 76:
> 
>> 74:     private static byte[] initialClassBytes;
>> 75: 
>> 76:     private static class VersionScanner extends ClassVisitor {
> 
> This class needs some explanation.
> It is unclear how does it work.
> Could you, please, add relevant comments where needed?
> For instance, how are the class file bytes constructed and what role does the version play.

Done.

> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java line 236:
> 
>> 234:                 redefine(2, 5);    // updates cached class bytes
>> 235:                 retransform(3, 2);
>> 236:                 retransform(4, 2);
> 
> The comments for all test cases need to be aligned.
> Also, it is better to comment all redefine/retransform cases.

Done.

> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/libRedefineRetransform.cpp line 179:
> 
>> 177:     if (err != JVMTI_ERROR_NONE) {
>> 178:         _log("nRedefine: SetEventNotificationMode(JVMTI_DISABLE) error %d\n", err);
>> 179:     }
> 
> It makes sense to introduce an utility function which does SetEventNotificationMode.
> It can be two separate functions to enable and disable or one single function can support both cases.

Removed code duplication - now the code is in helper class

> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/libRedefineRetransform.cpp line 195:
> 
>> 193:         _log("nRedefine: classLoadHookSavedClassBytes is NULL\n");
>> 194:         return nullptr;
>> 195:     }
> 
> The checks 181-195 seems to be the same in functions nRedefine and nRetransform,
> so one utility function can be used in both cases.

Removed code duplication - now the code is in helper class

> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/libRedefineRetransform.cpp line 274:
> 
>> 272:     classLoadHookSavedClassBytes = nullptr;
>> 273: 
>> 274:     return result;
> 
> The fragments 197-214 and 256-274 do the same.
> I'd suggest to define and use a function that does this post-processing.

reworked the code a bit, moved all common code to helper class

-------------

PR: https://git.openjdk.org/jdk/pull/10032