On Wed, 20 Mar 2024 04:40:52 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 107: >> >>> 105: newClassBytes = classBytes; >>> 106: fInst.retransformClasses(targetClass); >>> 107: assertTrue(targetClass.getName() + " was not seen by >>> transform()", seenClassBytes != null); >> >> Nit: I guess, the `targetClassName` was intended to be used in place of >> `targetClass.getName()`. > > 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1532968774
