Mixin local variable analysis misbehaving

[Su5eD]

Minecraft Version: 1.20.4

NeoForge Version: 20.4.218

Logs: https://mclo.gs/1JbVF64

---

A strange encounter

To give this a little background: It all started with a bug report I've received in Connector about a certain mod's mixin not applying. Specifically, it could not locate an implicit variable in its target method. After investigating the crash, I was left confused as the predicted local variables that should've been available at one of the mixin's injection points did not match mixin's analysis results. To make matters more curious, the target method wasn't modified by NeoForge at all. This led me to compare the results on both NeoForge and Fabric in an attempt to figure out what was wrong, with a surprising outcome.

I've verified this bug by implementing the same mixin on both NeoForge and Fabric to make sure this wasn't a side effect of Connector, but I figured I'd still mention it to explain how I came to compare Neo and Fabric Mixin behavior in the first place.

All of this comes down to a single method in Mixin, which is responsible for "guessing" the available local variables at a given injection point - Locals#getLocalsAt.
Let's take a closer look - below is my Mixin class I've used to reproduce this bug on NeoForged using (Fabric) Mixin version 0.13.0+mixin.0.8.5, although it's worth mentioning this applies to upstream Mixin as well.

@Mixin(LiquidBlock.class)  
public class LiquidBlockMixin {  

    @ModifyReturnValue(method = "shouldSpreadLiquid", at = @At("RETURN"))  
    private boolean thisMixinWontWorkNoMatterWhat(boolean original, @Local Level world, @Local BlockPos pos) {  
        return original;  
    }  
}

Get the complete minimal reproduction examples here:

• Forge
• Fabric (control)

Expected behavior

Below you can find an overview of our target method, net.minecraft.world.level.block.LiquidBlock#shouldSpreadLiquid.
There are 3 total return instructions, which gives 3 candidate injection points. To pass, the mixin must successfully inject into at least one of them. We are implicitly capturing a BlockPos and BlockState local variable. (The requirement for capturing implicit local variables being that there must only be one variable of the given type present at the injection point).

From the image below, we can observe that only one injection point out of three satisfies this requirement. The other two will fail to inject and remain ignored. Let's focus on the third return statement, which is our desired injection target for this reproduction case.

Actual behavior

At first glance, everything appears to be correct. When trying to launch the game, however, we are met with an unexpected crash. This happens due to all of the 3 possible injection points being rejected.

• Mixin crash log

Suppressed: com.llamalad7.mixinextras.sugar.impl.SugarApplicationException: Failed to validate sugar @Local BlockPos on method
Caused by: org.spongepowered.asm.mixin.injection.modify.InvalidImplicitDiscriminatorException: Found 2 candidate variables but exactly 1 is required.

What does Mixin have to say about locals at our injection point? I called the responsible method using the IDE debug evaluator to find several "ghost" variables present at the injection point, which shouldn't've existed. On Fabric, Mixin reported only 4 variables present - the class instance and 3 method arguments.

Show evaluation result

Curiously, despite the method in question being the same on both platforms, the mixin applies perfectly fine on Fabric, but breaks on Neo. Still, there was a chance Neo's recompilation of the class modified some of the variables' scopes or frames. To eliminate this possibility, I called the getLocalsAt method on the same instruction in a clean srg minecraft class, with the results being identical. At that point, I had eliminated any possibility this was caused by a difference in code.

Troublesome flags

I suspected this had something to do with how the method's frames are layed out, and after a while of poking around I've found out that the results are affected by whether the EXPAND_FRAMES flag is used when reading the ClassNode. And so, I went on to investigate the origin of ClassNodes used in the process.

Let's take a brief look at the contents of getLocalsAt:

public static LocalVariableNode[] getLocalsAt(ClassNode classNode, MethodNode method, AbstractInsnNode node, Settings settings) {  
//  ...
    ClassInfo classInfo = ClassInfo.forName(classNode.name);
// ...

There's a ClassNode classNode we're passing in as the input and a ClassInfo classInfo object, which holds pre-computed information about the class represented by classNode. The former is created by modlauncher in ClassTransformer#transform using 0 as the reader flags, while the ClassInfo is constructed using a ClassNode returned by mixins's byte code provider. In modlauncher, this happends to be MixinLaunchPluginLegacy#getClassNode, which uses ClassReader.EXPAND_FRAMES to read the class.

As it turns out, Fabric does the exact opposite:

• ClassNodes intended for transformation by mixin are parsed using ClassReader.EXPAND_FRAMES
• Classes returned by the bytecode provider use no flags

After testing with locally modified builds of modlauncher and mixin with the reader flags now set to match Fabric, the mixin injector applied just as expected.

Solutions

I'm not familiar with the inner workings of the getLocalsAt function, and looking at it's code, I'm not sure I want to be. However, I can make some conclusions based on my observations of its behavior:

• (our current setup) Using 0 for the input ClassNode and EXPAND_FRAMES for the ClassInfo node will produce inaccurate results with variables that are out of scope at the input instruction node.
• Using 0 for the input and 0 for ClassInfo can lead to unexpected ArrayIndexOutOfBoundsExceptions in some cases
• (our optimal choice) Using EXPAND_FRAMES for the input and 0 for ClassInfo node produces expected results

My proposal here is to update our bytecode provider and modlauncher's class reader to use the same flags as Fabric does for their ClassNodes. This might be a potentially breaking change and will require further testing to determine its impact on mods, but all things considered, if it works on Fabric, there's no reason it wouldn't for for us, too.