javassist icon indicating copy to clipboard operation
javassist copied to clipboard

CtConstructor.insertBefore yields VerifyError after transformation

Open kriegaex opened this issue 4 years ago • 22 comments

I am inserting code like this at the beginning of a constructor:

{
  int constructorStackDepth = dev.sarek.agent.constructor_mock.ConstructorMockRegistry.isMockUnderConstruction();
  if (constructorStackDepth > 0) {
    super(0);
    if (constructorStackDepth == 1) {
      dev.sarek.agent.constructor_mock.ConstructorMockRegistry.registerMockInstance($0);
    }
    return;
  }
}

This yields the following error:

java.lang.VerifyError
	at java.instrument/sun.instrument.InstrumentationImpl.retransformClasses0(Native Method)
	at java.instrument/sun.instrument.InstrumentationImpl.retransformClasses(InstrumentationImpl.java:167)
	at dev.sarek.agent.constructor_mock.JavassistConstructorMockIT.beforeClass(JavassistConstructorMockIT.java:62)

When dumping the byte code to a class file, IntelliJ's Fernflower decompiler can easily reproduce correct source code for my constructor:

  public Sub(int id, String name) {
    // This is the code inserted by Javassist transformation
    int var3 = ConstructorMockRegistry.isMockUnderConstruction();
    if (var3 > 0) {
      super(0);
      if (var3 == 1) {
        ConstructorMockRegistry.registerMockInstance(this);
      }
    }
    // This is the original constructor code before transformation
    else {
      super((new org.acme.Sub.Expensive(id)).getId());
      this.name = name;
      System.out.println("Constructing Sub -> " + this);
    }
  }

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

  • the ASM verifier (CheckClassAdapter.verify(..)) and
  • the BCEL verifier (Verifier.main(..)).

I got no clues what is wrong and makes the JVM verifier reject the class.

If I experimentally change

    if (constructorStackDepth == 1) {
      dev.sarek.agent.constructor_mock.ConstructorMockRegistry.registerMockInstance($0);
    }

right after the super call to just

      dev.sarek.agent.constructor_mock.ConstructorMockRegistry.registerMockInstance($0);

the 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.

kriegaex avatar Jul 07 '20 06:07 kriegaex

Okay, never mind, I found the problem's root cause. As I said, I was calling

int constructorStackDepth = dev.sarek.agent.constructor_mock.ConstructorMockRegistry.isMockUnderConstruction();

but in my JAR there was still an older version of class ConstructorMockRegistry in which isMockUnderConstruction() returned a boolean. Now it returns an int because it needs to report 3 types of states (2 before my change). Obviously I had only run mvn package on the dependency instead of mvn install, so my dependent module still picked up the older artifact.

I apologise for making unnecessary noise here, but the positive side-effect was that I learned about the ASM and BCEL byte code verifiers and also that they were unable to detect the problem I had because the problem actually was in the called class, not in the verified one. I was so convinced that I used Javassist wrong or found a bug therein, I only realised my mistake after I had ported the byte code generated by Javassist to ASM and there it was working . That made me double-check the dependency.

Bottom line: The problem was sitting in front of the keyboard. Shame on me!

kriegaex avatar Jul 07 '20 11:07 kriegaex

And again the situation has changed. The byte code generated by Javassist only worked because I had forgotten to remove -noverify from the Java command line. Now again it is not working. The ASM version is working, though. Trying to compare the byte code now.

kriegaex avatar Jul 07 '20 12:07 kriegaex

When I use javap -c -p -v on both class files, I see no differences at all in the generated byte code sequences, but in the stack map table. On the left hand side is the bogus Javassist byte code, on the right hand side the correct ASM byte code:

image

Question: Do I need to do anything else except insertBefore(..) in order to make Javassist update the stack map table for the local variable? The user manual does not say that I should. In ASM I do need to actively do something, but ASM is also more low-level than Javassist where I can just write Java source code as a string.


Update: I tried adding

ctConstructor.addLocalVariable("constructorStackDepth", CtPrimitiveType.intType);

and see one more entry in the local variable table now, but the VerifyError does not go away because still there is the difference you can see above in the second stack frame, frame type 0 (same) vs. 255 (full frame). I think Javassist needs to recompute the frame there correctly.

kriegaex avatar Jul 07 '20 12:07 kriegaex

@kriegaex Can you show us the full bytecode of that method you showed above? Just show us the output of javap -c -p -v. I suppose this is a kind of bug of stackmap generation. Thanks.

chibash avatar Jul 07 '20 17:07 chibash

Here is the full class incl. constant pool minus methods. The area of interest are the two constructors, anything else was not modified.

public class org.acme.Sub extends org.acme.Base
  minor version: 0
  major version: 52
  flags: ACC_PUBLIC, ACC_SUPER
Constant pool:
   #1 = Class              #2             // org/acme/Sub$Expensive
   #2 = Utf8               org/acme/Sub$Expensive
   #3 = Methodref          #1.#4          // org/acme/Sub$Expensive."<init>":(I)V
   #4 = NameAndType        #5:#6          // "<init>":(I)V
   #5 = Utf8               <init>
   #6 = Utf8               (I)V
   #7 = Methodref          #1.#8          // org/acme/Sub$Expensive.getId:()I
   #8 = NameAndType        #9:#10         // getId:()I
   #9 = Utf8               getId
  #10 = Utf8               ()I
  #11 = Methodref          #12.#4         // org/acme/Base."<init>":(I)V
  #12 = Class              #13       

     // org/acme/Base
  #13 = Utf8               org/acme/Base
  #14 = Fieldref           #15.#16        // org/acme/Sub.name:Ljava/lang/String;
  #15 = Class              #17            // org/acme/Sub
  #16 = NameAndType        #18:#19        // name:Ljava/lang/String;
  #17 = Utf8               org/acme/Sub
  #18 = Utf8               name
  #19 = Utf8               Ljava/lang/String;
  #20 = Fieldref           #21.#22        // java/lang/System.out:Ljava/io/PrintStream;
  #21 = Class              #23            // java/lang/System
  #22 = NameAndType        #24:#25        // out:Ljava/io/PrintStream;
  #23 = Utf8               java/lang/System
  #24 = Utf8               out
  #25 = Utf8               Ljava/io/PrintStream;
  #26 = Class              #27            // java/lang/StringBuilder
  #27 = Utf8               java/lang/StringBuilder
  #28 = Methodref          #26.#29        // java/lang/StringBuilder."<init>":()V
  #29 = NameAndType        #5:#30         // "<init>":()V
  #30 = Utf8               ()V
  #31 = String             #32            // Constructing Sub ->
  #32 = Utf8               Constructing Sub ->
  #33 = Methodref          #26.#34        // java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
  #34 = NameAndType        #35:#36        // append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
  #35 = Utf8               append
  #36 = Utf8               (Ljava/lang/String;)Ljava/lang/StringBuilder;
  #37 = Methodref          #26.#38        // java/lang/StringBuilder.append:(Ljava/lang/Object;)Ljava/lang/StringBuilder;
  #38 = NameAndType        #35:#39        // append:(Ljava/lang/Object;)Ljava/lang/StringBuilder;
  #39 = Utf8               (Ljava/lang/Object;)Ljava/lang/StringBuilder;
  #40 = Methodref          #26.#41        // java/lang/StringBuilder.toString:()Ljava/lang/String;
  #41 = NameAndType        #42:#43        // toString:()Ljava/lang/String;
  #42 = Utf8               toString
  #43 = Utf8               ()Ljava/lang/String;
  #44 = Methodref          #45.#46        // java/io/PrintStream.println:(Ljava/lang/String;)V
  #45 = Class              #47            // java/io/PrintStream
  #46 = NameAndType        #48:#49        // println:(Ljava/lang/String;)V
  #47 = Utf8               java/io/PrintStream
  #48 = Utf8               println
  #49 = Utf8               (Ljava/lang/String;)V
  #50 = Methodref          #15.#51        // org/acme/Sub."<init>":(ILjava/lang/String;)V
  #51 = NameAndType        #5:#52         // "<init>":(ILjava/lang/String;)V
  #52 = Utf8               (ILjava/lang/String;)V
  #53 = String             #54            // Sub@
  #54 = Utf8               Sub@
  #55 = Methodref          #56.#57        // java/lang/Object.hashCode:()I
  #56 = Class              #58            // java/lang/Object
  #57 = NameAndType        #59:#10        // hashCode:()I
  #58 = Utf8               java/lang/Object
  #59 = Utf8               hashCode
  #60 = Methodref          #26.#61        // java/lang/StringBuilder.append:(I)Ljava/lang/StringBuilder;
  #61 = NameAndType        #35:#62        // append:(I)Ljava/lang/StringBuilder;
  #62 = Utf8               (I)Ljava/lang/StringBuilder;
  #63 = String             #64            //  [name=
  #64 = Utf8                [name=
  #65 = String             #66            // , id=
  #66 = Utf8               , id=
  #67 = Fieldref           #15.#68        // org/acme/Sub.id:I
  #68 = NameAndType        #69:#70        // id:I
  #69 = Utf8               id
  #70 = Utf8               I
  #71 = String             #72            // ]
  #72 = Utf8               ]
  #73 = Utf8               Code
  #74 = Utf8               LineNumberTable
  #75 = Utf8               LocalVariableTable
  #76 = Utf8               this
  #77 = Utf8               Lorg/acme/Sub;
  #78 = Utf8               getName
  #79 = Utf8               SourceFile
  #80 = Utf8               Sub.java
  #81 = Utf8               InnerClasses
  #82 = Utf8               Expensive
  #83 = Utf8               dev/sarek/agent/constructor_mock/ConstructorMockRegistry
  #84 = Class              #83            // dev/sarek/agent/constructor_mock/ConstructorMockRegistry
  #85 = Utf8               isMockUnderConstruction
  #86 = NameAndType        #85:#10        // isMockUnderConstruction:()I
  #87 = Methodref          #84.#86        // dev/sarek/agent/constructor_mock/ConstructorMockRegistry.isMockUnderConstruction:()I
  #88 = Utf8               registerMockInstance
  #89 = Utf8               (Ljava/lang/Object;)V
  #90 = NameAndType        #88:#89        // registerMockInstance:(Ljava/lang/Object;)V
  #91 = Methodref          #84.#90        // dev/sarek/agent/constructor_mock/ConstructorMockRegistry.registerMockInstance:(Ljava/lang/Object;)V
  #92 = Utf8               java/lang/String
  #93 = Class              #92            // java/lang/String
  #94 = Utf8               StackMapTable
{
  protected final java.lang.String name;
    descriptor: Ljava/lang/String;
    flags: ACC_PROTECTED, ACC_FINAL

  public java.lang.String getName();
    descriptor: ()Ljava/lang/String;
    flags: ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: getfield      #14                 // Field name:Ljava/lang/String;
         4: areturn
      LineNumberTable:
        line 18: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       5     0  this   Lorg/acme/Sub;

  public org.acme.Sub(int, java.lang.String);
    descriptor: (ILjava/lang/String;)V
    flags: ACC_PUBLIC
    Code:
      stack=4, locals=4, args_size=3
         0: invokestatic  #87                 // Method dev/sarek/agent/constructor_mock/ConstructorMockRegistry.isMockUnderConstruction:()I
         3: istore_3
         4: iload_3
         5: iconst_0
         6: if_icmple     24
         9: aload_0
        10: iconst_0
        11: invokespecial #11                 // Method org/acme/Base."<init>":(I)V
        14: iload_3
        15: iconst_1
        16: if_icmpne     23
        19: aload_0
        20: invokestatic  #91                 // Method dev/sarek/agent/constructor_mock/ConstructorMockRegistry.registerMockInstance:(Ljava/lang/Object;)V
        23: return
        24: aload_0
        25: new           #1                  // class org/acme/Sub$Expensive
        28: dup
        29: iload_1
        30: invokespecial #3                  // Method org/acme/Sub$Expensive."<init>":(I)V
        33: invokevirtual #7                  // Method org/acme/Sub$Expensive.getId:()I
        36: invokespecial #11                 // Method org/acme/Base."<init>":(I)V
        39: aload_0
        40: aload_2
        41: putfield      #14                 // Field name:Ljava/lang/String;
        44: getstatic     #20                 // Field java/lang/System.out:Ljava/io/PrintStream;
        47: new           #26                 // class java/lang/StringBuilder
        50: dup
        51: invokespecial #28                 // Method java/lang/StringBuilder."<init>":()V
        54: ldc           #31                 // String Constructing Sub ->
        56: invokevirtual #33                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
        59: aload_0
        60: invokevirtual #37                 // Method java/lang/StringBuilder.append:(Ljava/lang/Object;)Ljava/lang/StringBuilder;
        63: invokevirtual #40                 // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
        66: invokevirtual #44                 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
        69: return
      LineNumberTable:
        line 7: 24
        line 8: 39
        line 9: 44
        line 10: 69
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      70     0  this   Lorg/acme/Sub;
            0      70     1    id   I
            0      70     2  name   Ljava/lang/String;
      StackMapTable: number_of_entries = 2
        frame_type = 255 /* full_frame */
          offset_delta = 23
          locals = [ class org/acme/Sub, int, class java/lang/String, int ]
          stack = []
        frame_type = 0 /* same */

  public org.acme.Sub(java.lang.String);
    descriptor: (Ljava/lang/String;)V
    flags: ACC_PUBLIC
    Code:
      stack=3, locals=3, args_size=2
         0: invokestatic  #87                 // Method dev/sarek/agent/constructor_mock/ConstructorMockRegistry.isMockUnderConstruction:()I
         3: istore_2
         4: iload_2
         5: iconst_0
         6: if_icmple     24
         9: aload_0
        10: iconst_0
        11: invokespecial #11                 // Method org/acme/Base."<init>":(I)V
        14: iload_2
        15: iconst_1
        16: if_icmpne     23
        19: aload_0
        20: invokestatic  #91                 // Method dev/sarek/agent/constructor_mock/ConstructorMockRegistry.registerMockInstance:(Ljava/lang/Object;)V
        23: return
        24: aload_0
        25: sipush        1234
        28: aload_1
        29: invokespecial #50                 // Method "<init>":(ILjava/lang/String;)V
        32: getstatic     #20                 // Field java/lang/System.out:Ljava/io/PrintStream;
        35: new           #26                 // class java/lang/StringBuilder
        38: dup
        39: invokespecial #28                 // Method java/lang/StringBuilder."<init>":()V
        42: ldc           #31                 // String Constructing Sub ->
        44: invokevirtual #33                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
        47: aload_0
        48: invokevirtual #37                 // Method java/lang/StringBuilder.append:(Ljava/lang/Object;)Ljava/lang/StringBuilder;
        51: invokevirtual #40                 // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
        54: invokevirtual #44                 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
        57: return
      LineNumberTable:
        line 13: 24
        line 14: 32
        line 15: 57
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      58     0  this   Lorg/acme/Sub;
            0      58     1  name   Ljava/lang/String;
      StackMapTable: number_of_entries = 2
        frame_type = 255 /* full_frame */
          offset_delta = 23
          locals = [ class org/acme/Sub, class java/lang/String, int ]
          stack = []
        frame_type = 0 /* same */

(... other methods ...)

SourceFile: "Sub.java"
InnerClasses:
     static #82= #1 of #15; //Expensive=class org/acme/Sub$Expensive of class org/acme/Sub

javap-Sub-javassist.txt javap-Sub-asm.txt

kriegaex avatar Jul 08 '20 00:07 kriegaex

BTW, this one might be similar to https://github.com/raphw/byte-buddy/issues/857#issuecomment-625465939 when I also tried to insert code in the "uninitialised this" part of a constructor, i.e. before super(), and the frame state was not corrected to "initialised". But this is just speculation, it was two months ago and I have not really compared. I only just remembered the common situation insert before super + stack map frame problem.

On a second thought, the problem occurs in the if condition after the super() call. So it is not the exact same thing, just similar.

kriegaex avatar Jul 08 '20 00:07 kriegaex

I understand that calling super() twice (at two places) is not valid according to the specification. However, if ASM and BCEL can generate a valid stackmap for that code, Javassist should be also able to do so.

chibash avatar Jul 08 '20 03:07 chibash

I understand that calling super() twice (at two places) is not valid according to the specification.

That is incorrect. Maybe you cannot do it from Java source code, just like you cannot put any source code before super() or this(). But from the JVM byte code perspective, as long as every execution path has exactly one super() call, it is legal to call it in several logical branches, which is what I do. Actually calling it twice from one control flow is illegal, of course. Also, in order to e.g. initialise parameters to super() or this(), it must be possible to execute code before the call.

However, if ASM and BCEL can generate a valid stackmap for that code, Javassist should be also able to do so.

I agree. That would be very nice.

kriegaex avatar Jul 08 '20 05:07 kriegaex

Although my memory might be incorrect, I believe the JVM specification (not Java lang. specification!) restricts what a constructor can do with uninitialized this before calling super(). But, yes, the real hotspot JVM is not strictly compatible with the JVM specification and thus we have to do reverse engineering to know what is allowed before calling super(). The specification of stackmaps is also very ambiguous. Again, reverse engineering is needed...

chibash avatar Jul 08 '20 05:07 chibash

I believe the JVM specification (not Java lang. specification!) restricts what a constructor can do with uninitialized this before calling super().

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 this if you want to check the byte code again. I am not using it as a method parameter, not trying to access any members or call any instance methods. I only call super(), as intended by the JVM specification. Only after the super() call, I use the (now already initialised) this as a method parameter when calling a static method in another class. So far, so legal.

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.

kriegaex avatar Jul 08 '20 06:07 kriegaex

BTW, if you like to talk more interactively, feel free to join Gitter and talk to me there.

kriegaex avatar Jul 08 '20 06:07 kriegaex

I never say what you're saying is shady! Your report is very valuable information that helps to improve Javassist's stackmap generator.

If the specification were very clear, the implementation would be straightforward because we could just implement as the spec. says. But the reality is not such an easy thing... The reports like yours are only the way to clarify the corner cases that the specification does not explicitly mention. So I'd like to thank you for this report.

I'll fix this but it might take time because I'm super busy these weeks (like others) with my daily jobs due to the COVID-19 issues.

chibash avatar Jul 08 '20 07:07 chibash

Let me ask one question. Is the output of javap you pasted above:

https://github.com/jboss-javassist/javassist/issues/328#issuecomment-655207465

the bytecode that ASM generated? Or the bytecode by Javassist? It looks like the code by ASM.

In the diff shown in:

https://github.com/jboss-javassist/javassist/issues/328#issuecomment-654819872

the right hand is ASM's, isn't it?

chibash avatar Jul 08 '20 07:07 chibash

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 diff them.

In the diff screenshot on the left is Javassist and on the right ASM, correct.

kriegaex avatar Jul 08 '20 08:07 kriegaex

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 insertBefore?

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):

  // access flags 0x1
  public <init>(Ljava/lang/String;)V

// Javassist-generated code here
    INVOKESTATIC dev/sarek/agent/constructor_mock/ConstructorMockRegistry.isMockUnderConstruction ()I
    ISTORE 2
    ILOAD 2
    ICONST_0
    IF_ICMPLE L0
    ALOAD 0
    ICONST_0
    INVOKESPECIAL org/acme/Base.<init> (I)V
    ILOAD 2
    ICONST_1
    IF_ICMPNE L1
    ALOAD 0
    INVOKESTATIC dev/sarek/agent/constructor_mock/ConstructorMockRegistry.registerMockInstance (Ljava/lang/Object;)V
   L1
   FRAME FULL [org/acme/Sub java/lang/String I] []
    RETURN

// Original code here
   L0
    LINENUMBER 13 L0
// Full frame (inserted by ASM, but not by Javassist, causing VerifyError)
   FRAME FULL [U java/lang/String I] []
    ALOAD 0
    SIPUSH 1234
    ALOAD 1
    INVOKESPECIAL org/acme/Sub.<init> (ILjava/lang/String;)V
   L2
    LINENUMBER 14 L2
    GETSTATIC java/lang/System.out : Ljava/io/PrintStream;
    NEW java/lang/StringBuilder
    DUP
    INVOKESPECIAL java/lang/StringBuilder.<init> ()V
    LDC "Constructing Sub -> "
    INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
    ALOAD 0
    INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/Object;)Ljava/lang/StringBuilder;
    INVOKEVIRTUAL java/lang/StringBuilder.toString ()Ljava/lang/String;
    INVOKEVIRTUAL java/io/PrintStream.println (Ljava/lang/String;)V
   L3
    LINENUMBER 15 L3
    RETURN
   L4
    LOCALVARIABLE this Lorg/acme/Sub; L0 L4 0
    LOCALVARIABLE name Ljava/lang/String; L0 L4 1
    MAXSTACK = 3
    MAXLOCALS = 3

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.

kriegaex avatar Jul 11 '20 14:07 kriegaex

Sure. Javassist automatically constructs a stackmap from the bare bytecode. Please look at javassist.bytecode.stackmap. It first constructs a control flow graph and computes a data flow on that graph so that it will infer the types of stack elements and local variables. I suppose that there is a bug to infer the type of the local variable 0 (= this); when it is initialized.

BTW, how did you get the correct stackmap by ASM? I'm just curious.

chibash avatar Jul 11 '20 17:07 chibash

Javassist automatically constructs a stackmap from the bare bytecode. Please look at javassist.bytecode.stackmap.

I will, thank you.

It first constructs a control flow graph and computes a data flow on that graph so that it will infer the types of stack elements and local variables. I suppose that there is a bug to infer the type of the local variable 0 (= this); when it is initialized.

BTW, how did you get the correct stackmap by ASM? I'm just curious.

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 -noverify now. Of course that is inefficient and only a temporary workaround, hence the TODO in my code.

kriegaex avatar Jul 12 '20 01:07 kriegaex

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

  1. is generic enough to cover all cases where after branching in a constructor emitting a full frame due to the need for an uninitialised this is necessary and
  2. does not break anything else anywhere else.

So please be sceptical and verify this. If you think I solved the problem, I can send a PR instead of an inline patch.

kriegaex avatar Jul 12 '20 04:07 kriegaex

Very close! Although your suggestion is not an exact one, it helped me lots. Thanks.

chibash avatar Jul 13 '20 00:07 chibash

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.

kriegaex avatar Jul 13 '20 10:07 kriegaex

I do not have any plan for release. Maybe releasing 3.28 in (late) August?

chibash avatar Jul 13 '20 16:07 chibash

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...

kriegaex avatar Jul 14 '20 02:07 kriegaex