On Wed, 20 Mar 2024 22:24:37 GMT, Alex Menkov <amen...@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
