Merge branch '317827-compute-maxs-from-frames' into 'master'

Resolve "Computing maximum stack size does not work correctly with dead code present"

Closes #317827

See merge request !173

asm/src/main/java/org/objectweb/asm/ClassWriter.java

@@ -45,6 +45,11 @@ public class ClassWriter extends ClassVisitor {
    * #visitMethod} method will be ignored, and computed automatically from the signature and the
    * bytecode of each method.
    *
+   * <p><b>Note:</b> for classes whose version is {@link Opcodes#V1_7} of more, this option requires
+   * valid stack map frames. The maximum stack size is then computed from these frames, and from the
+   * bytecode instructions in between. If stack map frames are not present or must be recomputed,
+   * used {@link #COMPUTE_FRAMES} instead.
+   *
    * @see #ClassWriter(int)
    */
   public static final int COMPUTE_MAXS = 1;
@@ -266,6 +271,9 @@ public class ClassWriter extends ClassVisitor {
         this.interfaces[i] = symbolTable.addConstantClass(interfaces[i]).index;
       }
     }
+    if (compute == MethodWriter.COMPUTE_MAX_STACK_AND_LOCAL && (version & 0xFFFF) >= Opcodes.V1_7) {
+      compute = MethodWriter.COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES;
+    }
   }
 
   @Override

asm/src/main/java/org/objectweb/asm/MethodWriter.java

@@ -38,11 +38,22 @@ package org.objectweb.asm;
  */
 final class MethodWriter extends MethodVisitor {
 
+  /** Indicates that nothing must be computed. */
+  static final int COMPUTE_NOTHING = 0;
+
   /**
-   * Indicates that all the stack map frames must be computed. In this case the maximum stack size
-   * and the maximum number of local variables is also computed.
+   * Indicates that the maximum stack size and the maximum number of local variables must be
+   * computed, from scratch.
    */
-  static final int COMPUTE_ALL_FRAMES = 3;
+  static final int COMPUTE_MAX_STACK_AND_LOCAL = 1;
+
+  /**
+   * Indicates that the maximum stack size and the maximum number of local variables must be
+   * computed, from the existing stack map frames. This can be done more efficiently than with the
+   * control flow graph algorithm used for {@link #COMPUTE_MAX_STACK_AND_LOCAL}, by using a linear
+   * scan of the bytecode instructions.
+   */
+  static final int COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES = 2;
 
   /**
    * Indicates that the stack map frames of type F_INSERT must be computed. The other frames are not
@@ -50,16 +61,13 @@ final class MethodWriter extends MethodVisitor {
    * the F_INSERT frames, together with the bytecode instructions between a F_NEW and a F_INSERT
    * frame - and without any knowledge of the type hierarchy (by definition of F_INSERT).
    */
-  static final int COMPUTE_INSERTED_FRAMES = 2;
+  static final int COMPUTE_INSERTED_FRAMES = 3;
 
   /**
-   * Indicates that the maximum stack size and the maximum number of local variables must be
-   * computed.
+   * Indicates that all the stack map frames must be computed. In this case the maximum stack size
+   * and the maximum number of local variables is also computed.
    */
-  static final int COMPUTE_MAX_STACK_AND_LOCAL = 1;
-
-  /** Indicates that nothing must be computed. */
-  static final int COMPUTE_NOTHING = 0;
+  static final int COMPUTE_ALL_FRAMES = 4;
 
   /** Indicates that {@link #STACK_SIZE_DELTA} is not applicable (not constant or never used). */
   private static final int NA = 0;
@@ -471,11 +479,12 @@ final class MethodWriter extends MethodVisitor {
 
   /**
    * The current basic block, i.e. the basic block of the last visited instruction. When {@link
-   * #compute} is equal to {@link #COMPUTE_ALL_FRAMES}, this field is <tt>null</tt> for unreachable
-   * code. When {@link #compute} is equal to {@link #COMPUTE_INSERTED_FRAMES}, this field stays
+   * #compute} is equal to {@link #COMPUTE_MAX_STACK_AND_LOCAL} or {@link #COMPUTE_ALL_FRAMES}, this
+   * field is <tt>null</tt> for unreachable code. When {@link #compute} is equal to {@link
+   * #COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES} or {@link #COMPUTE_INSERTED_FRAMES}, this field stays
    * unchanged throughout the whole method (i.e. the whole code is seen as a single basic block;
-   * indeed, the existing frames are sufficient by hypothesis to compute any intermediate frame
-   * without using any control flow graph).
+   * indeed, the existing frames are sufficient by hypothesis to compute any intermediate frame -
+   * and the maximum stack size as well - without using any control flow graph).
    */
   private Label currentBasicBlock;
 
@@ -483,7 +492,10 @@ final class MethodWriter extends MethodVisitor {
    * The relative stack size after the last visited instruction. This size is relative to the
    * beginning of {@link #currentBasicBlock}, i.e. the true stack size after the last visited
    * instruction is equal to the {@link Label#inputStackSize} of the current basic block plus {@link
-   * #relativeStackSize}.
+   * #relativeStackSize}. When {@link #compute} is equal to {@link
+   * #COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES}, {@link #currentBasicBlock} is always the start of
+   * the method, so this relative size is also equal to the absolute stack size after the last
+   * visited instruction.
    */
   private int relativeStackSize;
 
@@ -491,7 +503,10 @@ final class MethodWriter extends MethodVisitor {
    * The maximum relative stack size after the last visited instruction. This size is relative to
    * the beginning of {@link #currentBasicBlock}, i.e. the true maximum stack size after the last
    * visited instruction is equal to the {@link Label#inputStackSize} of the current basic block
-   * plus {@link #maxRelativeStackSize}.
+   * plus {@link #maxRelativeStackSize}.When {@link #compute} is equal to {@link
+   * #COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES}, {@link #currentBasicBlock} is always the start of
+   * the method, so this relative size is also equal to the absolute maximum stack size after the
+   * last visited instruction.
    */
   private int maxRelativeStackSize;
 
@@ -833,6 +848,13 @@ final class MethodWriter extends MethodVisitor {
       ++stackMapTableNumberOfEntries;
     }
 
+    if (compute == COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES) {
+      relativeStackSize = nStack;
+      if (nStack > maxRelativeStackSize) {
+        maxRelativeStackSize = relativeStackSize;
+      }
+    }
+
     maxStack = Math.max(maxStack, nStack);
     maxLocals = Math.max(maxLocals, currentLocals);
   }
@@ -1135,6 +1157,9 @@ final class MethodWriter extends MethodVisitor {
         }
       } else if (compute == COMPUTE_INSERTED_FRAMES) {
         currentBasicBlock.frame.execute(baseOpcode, 0, null, null);
+      } else if (compute == COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES) {
+        // No need to update maxRelativeStackSize (the stack size delta is always negative).
+        relativeStackSize += STACK_SIZE_DELTA[baseOpcode];
       } else {
         if (baseOpcode == Opcodes.JSR) {
           // Record the fact that 'label' designates a subroutine, if not already done.
@@ -1241,6 +1266,11 @@ final class MethodWriter extends MethodVisitor {
         lastBasicBlock.nextBasicBlock = label;
       }
       lastBasicBlock = label;
+    } else if (compute == COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES && currentBasicBlock == null) {
+      // This case should happen only once, for the visitLabel call in the constructor. Indeed, if
+      // compute is equal to COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES, currentBasicBlock stays
+      // unchanged.
+      currentBasicBlock = label;
     }
   }
 
@@ -1337,7 +1367,7 @@ final class MethodWriter extends MethodVisitor {
           addSuccessorToCurrentBasicBlock(Edge.JUMP, label);
           label.getCanonicalInstance().flags |= Label.FLAG_JUMP_TARGET;
         }
-      } else {
+      } else if (compute == COMPUTE_MAX_STACK_AND_LOCAL) {
         // No need to update maxRelativeStackSize (the stack size delta is always negative).
         --relativeStackSize;
         // Add all the labels as successors of the current basic block.
@@ -1510,6 +1540,8 @@ final class MethodWriter extends MethodVisitor {
       computeAllFrames();
     } else if (compute == COMPUTE_MAX_STACK_AND_LOCAL) {
       computeMaxStackAndLocal();
+    } else if (compute == COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES) {
+      this.maxStack = maxRelativeStackSize;
     } else {
       this.maxStack = maxStack;
       this.maxLocals = maxLocals;
@@ -1687,7 +1719,7 @@ final class MethodWriter extends MethodVisitor {
     // graph, and add these blocks to the list of blocks to process (if not already done).
     Label listOfBlocksToProcess = firstBasicBlock;
     listOfBlocksToProcess.nextListElement = Label.EMPTY_LIST;
-    int maxStackSize = 0;
+    int maxStackSize = maxStack;
     while (listOfBlocksToProcess != Label.EMPTY_LIST) {
       // Remove a basic block from the list of blocks to process. Note that we don't reset
       // basicBlock.nextListElement to null on purpose, to make sure we don't reprocess already
@@ -1722,7 +1754,7 @@ final class MethodWriter extends MethodVisitor {
         outgoingEdge = outgoingEdge.nextEdge;
       }
     }
-    this.maxStack = Math.max(maxStack, maxStackSize);
+    this.maxStack = maxStackSize;
   }
 
   @Override
@@ -1759,10 +1791,9 @@ final class MethodWriter extends MethodVisitor {
       nextBasicBlock.resolve(code.data, code.length);
       lastBasicBlock.nextBasicBlock = nextBasicBlock;
       lastBasicBlock = nextBasicBlock;
-    } else {
+      currentBasicBlock = null;
+    } else if (compute == COMPUTE_MAX_STACK_AND_LOCAL) {
       currentBasicBlock.outputStackMax = (short) maxRelativeStackSize;
-    }
-    if (compute != COMPUTE_INSERTED_FRAMES) {
       currentBasicBlock = null;
     }
   }

asm/src/test/java/org/objectweb/asm/ClassWriterComputeMaxsTest.java

@@ -56,8 +56,12 @@ public class ClassWriterComputeMaxsTest {
 
   @BeforeEach
   public void setUp() throws Exception {
+    init(Opcodes.V1_1);
+  }
+
+  private void init(final int classVersion) {
     classWriter = new ClassWriter(ClassWriter.COMPUTE_MAXS);
-    classWriter.visit(Opcodes.V1_1, Opcodes.ACC_PUBLIC, "C", null, "java/lang/Object", null);
+    classWriter.visit(classVersion, Opcodes.ACC_PUBLIC, "C", null, "java/lang/Object", null);
     methodVisitor = classWriter.visitMethod(Opcodes.ACC_PUBLIC, "<init>", "()V", null, null);
     methodVisitor.visitCode();
     methodVisitor.visitVarInsn(Opcodes.ALOAD, 0);
@@ -1101,4 +1105,27 @@ public class ClassWriterComputeMaxsTest {
     assertMaxs(1, 3);
     assertGraph("N0=N18", "N3=N4", "N4=N21", "N6=N3,N4", "N14=", "N18=N6", "N21=");
   }
+
+  /**
+   * Tests computing the maximum stack size from the existing stack map frames and the instructions
+   * in between, when dead code is present.
+   */
+  @Test
+  public void testComputeMaxsFromFramesWithDeadCode() {
+    init(Opcodes.V1_7);
+    Label l0 = new Label();
+
+    RETURN();
+    // With the default compute maxs algorithm, this dead code block is not considered for the
+    // maximum stack size, which works fine for classes up to V1_6. Starting with V1_7, stack map
+    // frames are mandatory, even for dead code, and the maximum stack size must take dead code into
+    // account. Hopefully it can be computed from the stack map frames, and the instructions in
+    // between (without any control flow graph construction or algorithm).
+    LABEL(l0);
+    methodVisitor.visitFrame(Opcodes.F_SAME, 0, null, 0, null);
+    ACONST_NULL();
+    RETURN();
+
+    assertMaxs(1, 1);
+  }
 }