Synthetic label generation is off-by-one in fake LVT generation

[Earthcomputer]

This is quite a niche scenario, as it requires a few rare things to coincide:

1. The variable you are targeting has no LVT entry
2. The AbstractInsnNode after the variable store instruction is not a label node
3. The variable you're targeting is not stored again after the store which satisfies condition 2.

Normally, condition 2 isn't satisfied, because normally a variable store is the last instruction in a compiled Java statement.

This has been encountered in the wild in legacy Minecraft versions where there are no LVTs. Take for example the ScreenshotUtils class from Beta 1.7.3:

public class ScreenshotUtils {
   public static void saveScreenshot(File gameDir, int width, int height) {
      // ...
      File var3 = new File(gameDir, "screenshots");
      // ...
      File var5;
      while ((var5 = new File(var3, var4 + (var6 == 1 ? "" : "_" + var6) + ".png")).exists()) {
          var6++;
      }
      // ...
      var15.setRGB(0, 0, width, height, f_7482814, 0, width);
      // ...
   }
}

and the mixin:

@Mixin(ScreenshotUtils.class)
public class ScreenshotUtilsMixin {
    @ModifyVariable(method = "saveScreenshot", at = @At(value = "INVOKE", target = "Ljava/awt/image/BufferedImage;setRGB(IIII[III)V"), ordinal = 2)
    private static File test(File file) {
        return file;
    }
}

Mixin is unable to find the local variable and fails. Printing the local variables verifies that var5 is missing even despite being used after the setRGB call.

After digging fairly deep into this issue, I noticed a problem in the fallback fake LVT generation for when the local variable is not found in the real LVT. Sometimes, Mixin needs to add synthetic labels into the method node if the labels are not already there, and it seems to be adding them one instruction later than they are supposed to be. From the example above, textifying the bytecode around the inline assignment of var5 shows that there is a label, L19, which is one instruction later than expected (var5 is at index 5):

...
    INVOKEVIRTUAL java/lang/StringBuilder.toString ()Ljava/lang/String;
    INVOKESPECIAL java/io/File.<init> (Ljava/io/File;Ljava/lang/String;)V
    DUP
    ASTORE 5
    INVOKEVIRTUAL java/io/File.exists ()Z
   L19
    IFEQ L20
...

This causes the code inside Locals.getLocalsAt to not know what the type of the local variable being stored by ASTORE 5 is, because the local variable it needs to find is only alive after the L19 label. Therefore, Mixin removes this local variable from the frame (at the end of Locals.getLocalsAt).

I believe all this is caused by this line. I think it should be insertBefore, rather than insert. I would double check that of course though, as I didn't dig any deeper than this.