RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v5]

[Serguei Spitsyn]

On Wed, 20 Mar 2024 22:24:37 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

>> The `retransform()` method returns an array of bytes. There is also a comment about it at the line 101. But the result has never been checked (please, see lines: 116, 123, 129, 135). This creates some confusion and questions why the return value is needed.
>> The parameter `null` at lines 123, 129, 135 is also confusing and need some explanation.
>> As I read the code, the method  `retransform()` is saving the parameter `classBytes` in the `newClassBytes` field. The field `newClassBytes` value is then returned by the `transform()` method.
>> But the `ClassFileTransformer.html#transform` states this:
>> `Returns: a well-formed class file buffer (the result of the transform), or null if no transform is performed`.
>> 
>> So that, when the `null` is returned by the `transform()` then it means there was no real transform.
>> Could you comment on this? Do I miss anything here?
>> 
>> Please, see: [ClassFileTransformer.html#transform](https://docs.oracle.com/en/java/javase/21/docs/api/java.instrument/java/lang/instrument/ClassFileTransformer.html#transform(java.lang.Module,java.lang.ClassLoader,java.lang.String,java.lang.Class,java.security.ProtectionDomain,byte%5B%5D))
>> 
>> Also, the comment at line 122 is not needed at this form:
>> 
>>             // Ensure retransformation does not fail with ClassFormatError.
>> 
>> Sorry, I was not clear when I've asked to add a comment at this point. I wanted a clarification about what does passing the `null` mean in terms of transformation. My understanding is that there is no real transformation when the `null` is returned but the `Instrumentation` mechanism is still working (for instance, the `JvmtiClassFileReconstituter` can be involved with an incorrect attributes counter) and can fail and we want to test/verify it.  So, passing a Null-transformer is enough for our testing. Is it correct?
>> Please, keep in mind, few people have some knowledge about the re-transformation details and would appreciate any hints here.
>
> Instrumentation.retransformClasses logic can be described by the following pseudo-code:
> 
> byte[] newClassBytes = JvmtiClassFileReconstituter.reconstitute(klass);
> for (Transformer tr = firstTransformer(); tr != null; tr = tr->next()) {
>   byte[] transformerClassBytes = tr.transform(newClassBytes);
>   if (newClassBytes != null) {
>     newClassBytes = transformerClassBytes;
>   }
> }
> 
> Then newClassBytes is parsed, verified and is used to redefine the class.
> 
> We need to verify that JvmtiClassFileReconstituter produces correct class bytes, so test transformer can return null or passed classfileBuffer.
> Actually the issue can be reproduced without ClassFileTransformer at all (and this is mentioned in the CR) - in the case reconstituted class bytes is considered as a result of the instrumentation.
> 
> I've updated the test, hope it's clearer now.
> `targetClassName`/`seenClassBytes`/`newClassBytes` was used by Transformer, I moved them to the class.
> Please let me know if you think some additional comments would be useful.

Thank you for the update, it is a nice change.
I'd also suggest to add a comment before line 115, something like this:

  // Below the null is passed as the classBytes argument which means there won't be any transformation.
  // However, it is enough for testing purposes as the JvmtiClassFileReconstituter still involved
  // into preparation of the initial classfile bytecodes.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1533251745