-
Notifications
You must be signed in to change notification settings - Fork 698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CtConstructor.insertBefore yields VerifyError after transformation #328
Comments
This comment has been minimized.
This comment has been minimized.
And again the situation has changed. The byte code generated by Javassist only worked because I had forgotten to remove |
When I use Question: Do I need to do anything else except Update: I tried adding ctConstructor.addLocalVariable("constructorStackDepth", CtPrimitiveType.intType); and see one more entry in the local variable table now, but the |
@kriegaex Can you show us the full bytecode of that method you showed above? |
Here is the full class incl. constant pool minus methods. The area of interest are the two constructors, anything else was not modified.
|
BTW, this one might be similar to raphw/byte-buddy#857 (comment) when I also tried to insert code in the "uninitialised this" part of a constructor, i.e. before On a second thought, the problem occurs in the |
I understand that calling super() twice (at two places) is not valid according to the specification. |
…n callback I implemented this in Javassist first, then migrated the solution to ASM + BB. Actually, the Javassist version only works with '-noverify' because there of Javassist bug jboss-javassist/javassist#328. Status quo: - ConstructorMockRegistry.isMockUnderConstruction() now returns an int instead of a boolean. Negative value means: no mock under construction. Positive values reflect the stack depth of constructors, i.e. 1 means that the top-level constructor (TLC) is currently being executed. This is utilised in order to only call ConstructorMockRegistry.registerMockInstance(this) after the super() returns to the TLC. - ConstructorMockRegistry.mockInstances is a private static Set<Object> in which all new object instances are registered, but it is not being used anywhere yet and does not even have an accessor method. TODO: * 'unregister' method * automatic unregistration if mock class is deactivated * grouping instances by class or class name -> turn the Set into a Map
That is incorrect. Maybe you cannot do it from Java source code, just like you cannot put any source code before
I agree. That would be very nice. |
Although my memory might be incorrect, I believe the JVM specification (not Java lang. specification!) restricts what a constructor can do with uninitialized |
Of course there are restrictions, but that is not the point because I am not breaking them. I am not doing anything with the uninitialised Sorry to hear that the specs are not as clear as you wish them to be and you feel you have to reverse-engineer anything. Except for the part that I was just commenting on and had informed myself about beforehand by talking to other developers much more experienced than I in all things JVM (so I am confident to comment), I am sure you are 50x deeper into the JVM than I will ever be, and you have my deep respect for that. I just want to avoid you from thinking that what I am doing is shady or against the specs in any way, because I believe it is not. |
BTW, if you like to talk more interactively, feel free to join Gitter and talk to me there. |
I never say what you're saying is shady! If the specification were very clear, the implementation would be straightforward because we could just implement I'll fix this but it might take time because I'm super busy these weeks (like others) with my daily jobs |
Let me ask one question. the bytecode that ASM generated? Or the bytecode by Javassist? In the diff shown in: the right hand is ASM's, isn't it? |
Oh, I copied from the wrong side of the diff, my bad. Good catch! I updated the inline code block to show the Javassist version with the stack frame problem and also attached both text files to the comment, see above. The only thing I changed is the ordering of elements in the ASM output (e.g. location of stack frames in the file) in order to make it easy to In the diff screenshot on the left is Javassist and on the right ASM, correct. |
Would you mind giving me a hint as to where in your code the places are where you would look at and what roughly you think you would do in order to insert the extra full frame at the beginning of the original code section, i.e. after the end of the code I inserted via In ASM syntax, this is missing // My final RETURN after my own super() call + subsequent logic
methodVisitor.visitInsn(RETURN);
// Start of original constructor code
methodVisitor.visitLabel(label0);
methodVisitor.visitLineNumber(13, label0);
// Full frame (inserted by ASM, but not by Javassist, causing VerifyError)
methodVisitor.visitFrame(Opcodes.F_FULL, 3, new Object[] { Opcodes.UNINITIALIZED_THIS, "java/lang/String", Opcodes.INTEGER }, 0, new Object[] {});
// Load uninitialised this and the rest of the original constructor code...
methodVisitor.visitVarInsn(ALOAD, 0);
// ... Or in mnemonic syntax (full method):
I am not saying I could prepare a proper pull request, but I like to learn and take a look at least and maybe play around a bit if I had a starting point where to look and how to make Javassist insert a different frame type. I understand if you are too busy, but if it is not too time-consuming to explain I would be glad to listen and learn. |
Sure. Javassist automatically constructs a stackmap from the bare bytecode. Please look at BTW, how did you get the correct stackmap by ASM? I'm just curious. |
I will, thank you.
I am not generating fames by myself but let ASM do it for me. I guess the way it does it is basically the same method you described for Javassist. BTW, our talk inspired me to try this: // After Javassist transformation
transformedBytecode = targetClass.toBytecode();
// Repair stack map frames via ASM
// TODO: remove after fix for https://github.com/jboss-javassist/javassist/issues/328 is released
ClassReader classReader = new ClassReader(transformedBytecode);
ClassWriter classWriter = new ClassWriter(ClassWriter.COMPUTE_FRAMES);
classReader.accept(classWriter, ClassReader.SKIP_FRAMES);
transformedBytecode = classWriter.toByteArray(); Works like a charm. I.e., I let ASM repair the Javassist-generated stack map until this issue is fixed. My subsequent tests run without |
Disclaimer: I am still pretty much a beginner in byte code instrumentation and almost a noob when it comes to the JVM opcode level. So please don't kill me if I am suggesting something stupid. I took a look at the package you told me, debugged a little bit and came up with this: Index: src/main/javassist/bytecode/stackmap/MapMaker.java
<+>UTF-8
===================================================================
--- src/main/javassist/bytecode/stackmap/MapMaker.java (revision 5e3b6a8f64a291ac98f1b50cb82da4cc1cff8f90)
+++ src/main/javassist/bytecode/stackmap/MapMaker.java (date 1594525544956)
@@ -459,8 +459,10 @@
int stackTop = bb.stackTop;
if (stackTop == 0) {
if (diffL == 0) {
- writer.sameFrame(offsetDelta);
- return;
+ if (bb.numLocals > 0 && !(bb.localsTypes[0].isUninit() && bb.localsTypes[0].getTypeTag() == StackMapTable.THIS)) {
+ writer.sameFrame(offsetDelta);
+ return;
+ }
}
else if (0 > diffL && diffL >= -3) {
writer.chopFrame(offsetDelta, -diffL); This solves the problem for my specific case and all Javassist tests are still passing. But of course I am not sure if I understand stack map generation in general and the way Javassist does it in particular well enough in order to make sure that this modification
So please be sceptical and verify this. If you think I solved the problem, I can send a PR instead of an inline patch. |
Very close! Although your suggestion is not an exact one, it helped me lots. Thanks. |
Thanks for the quick bugfix. I will build it locally and give you feedback. I saw that you adjusted the version number in the read-me and one more file, but not in the Maven POM. That tells me that you have not planned an immediate bugfix release. Am I correct? I am just asking because I want to decide whether to deploy a non-snapshot release locally with a special version number or wait for a release. BTW, funny detail: Version 3.28 would coindice with the fix of issue #328, same numbers. One more reason to release. 😉 Update: My first quick test is still passing after the build with your master. |
I do not have any plan for release. Maybe releasing 3.28 in (late) August? |
Okay, for now I just pushed a bugfix release to Maven Central with these Maven coordinates in order to avoid having to refer to a local snapshot version in my POM: <dependency>
<groupId>de.scrum-master.org.javassist</groupId>
<artifactId>javassist</artifactId>
<version>3.27.0-GA-bugfix-328</version>
</dependency> This way you do not have any time pressure and I do not need to wait. So in the improbable case that someone else also wants to use the bugfix before the next official release is out... |
…verify' TODO: remove after fix for #328 is released, see jboss-javassist/javassist#328
…ordinates GAV: de.scrum-master.org.javassist:javassist:3.27.0-GA-bugfix-328 I pushed this to Maven Central myself today because the Javassist maintainer does not plan a new release before end of 2020-08. Until then I do not wish to rely on a local snapshot if I share the Sarek repository with anyone else. TODO: - Remove workaround after fix for #328 is released, see jboss-javassist/javassist#328 - The ASM workaround also still is in the code base and the dependency in the POM. Either move the "repair stack map" method into a tool class or just delete it, then also get rid of the ByteBuddy dependency.
jboss-javassist/javassist#305 jboss-javassist/javassist#328 jboss-javassist/javassist#339 jboss-javassist/javassist#350 jboss-javassist/javassist#357 jboss-javassist/javassist#363 Change-Id: I29963013cf637731fe1064425b9d2e80d63bd9d3 Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
jboss-javassist/javassist#305 jboss-javassist/javassist#328 jboss-javassist/javassist#339 jboss-javassist/javassist#350 jboss-javassist/javassist#357 jboss-javassist/javassist#363 Change-Id: I29963013cf637731fe1064425b9d2e80d63bd9d3 Signed-off-by: Robert Varga <robert.varga@pantheon.tech> (cherry picked from commit 0df0ba3)
jboss-javassist/javassist#305 jboss-javassist/javassist#328 jboss-javassist/javassist#339 jboss-javassist/javassist#350 jboss-javassist/javassist#357 jboss-javassist/javassist#363 Change-Id: I29963013cf637731fe1064425b9d2e80d63bd9d3 Signed-off-by: Robert Varga <robert.varga@pantheon.tech> (cherry picked from commit 0df0ba3)
I am inserting code like this at the beginning of a constructor:
This yields the following error:
When dumping the byte code to a class file, IntelliJ's Fernflower decompiler can easily reproduce correct source code for my constructor:
I do not know why the JVM verifier complains. Maybe there is something wrong with local variable tables, stack map frames or whatever low-level byte code stuff I don't know enough about in order to analyse the problem.
I successfully verified my transformed class with both
CheckClassAdapter.verify(..)
) andVerifier.main(..)
).I got no clues what is wrong and makes the JVM verifier reject the class.
If I experimentally change
right after the
super
call to justthe class gets transformed successfully, but of course it does not do the right thing anymore because the method call is conditional.
BTW, the
VerifyError
occurs both with Java 8 and Java 14 (probably also with all versions in between). If I run my test on the transformation rejected by the JVM verifier with-noverify
, it works nicely and does the right thing.The text was updated successfully, but these errors were encountered: