Prevent FieldAccessExpr in Elimination

[leslieyip02]

Fix decompilation of field updates by avoiding artificial SSA aliases, removing redundant self-assignments, and making expression elimination conservative about side effects and exceptions.

Bug

Given:

class Foo {
    private int value;

    void increment(int delta) {
        value += delta;
        return;
    }
}

Jade previously produced:

/**
 * Source File: Foo.java
 * Class-file Format Version: 63
 * Source Debug Extension: null // See JSR-45 https://www.jcp.org/en/jsr/detail?id=045
 */
class Foo extends java.lang.Object {

    private int value;

    Foo() {
        super();
        return;
    }

    void increment(int parameterVar2) {
        copyVar3_1.value = insnVar6;
        return;
    }
}

Both copyVar3_1 and insnVar6 were undeclared.

There were two underlying causes:

1. Duplicated variables

JVM stack rearrangement instructions such as DUP created new SSA copy variables even though they only duplicate or move an existing value. This results in extra copies of variables:

{
    Foo copyVar2_1;
    Foo copyVar3_2;
    int insnVar4;
    int copyVar5_1;
    int insnVar6;
    /*Instruction(basicValue=null, insn=7:PUTFIELD Foo.value : I (FIELD))*/
    ;
    {
        JADE_BLOCK_10: {
            JADE_BLOCK_9: {
                JADE_BLOCK_8: {
                    JADE_BLOCK_7: {
                        JADE_BLOCK_6: {
                            JADE_BLOCK_5: {
                                JADE_BLOCK_4: {
                                    JADE_BLOCK_3: {
                                        JADE_BLOCK_2: {
                                            JADE_BLOCK_1: /*Label: L1943750504*/
                                            ;
                                            /*Line number: 5*/
                                            ;
                                        }
                                        {
                                            copyVar2_1 = this;
                                        }
                                    }
                                    /*Operand Stack Operation: StackOperation(insn=org.objectweb.asm.tree.InsnNode@30f5a68a)*/
                                    ;
                                }
                                {
                                    insnVar4 = copyVar3_2.value;
                                }
                            }
                            {
                                copyVar5_1 = parameterVar2;
                            }
                        }
                        {
                            insnVar6 = insnVar4 + copyVar5_1;
                        }
                    }
                    copyVar3_1.value = insnVar6;
                }
                /*Label: L1907604549*/
                ;
            }
            /*Line number: 6*/
            ;
        }
        return;
    }
}

Jade assigns insnVar4 = copyVar3_2.value, but then also assigns copyVar3_1.value = insnVar6. The this variable was duplicated even though we are still referring to the same this.

2. Elimination was too aggressive

Elimination retains field assignments but do not mark its receiver and value as live, allowing their declarations to be removed.

jade/src/main/kotlin/org/ucombinator/jade/decompile/Elimination.kt

Lines 174 to 186 in 844706a

	fun backTransfer(statement: Statement, state: Set<String>): Set<String> {
	val res = state.toMutableSet() // live vars going out to successors
	when (statement) {
	is ExpressionStmt -> {
	when (val expr = statement.expression) {
	is AssignExpr -> {
	val target = expr.target
	if (target is NameExpr && res.contains(target.nameAsString)) {
	// res.remove(target.nameAsString)
	// vars used on rhs are live
	res.addAll(processVars(expr.value))
	}
	}

In the logic, only NameExpr is preserved. In the Foo example above, there is a FieldAccessExpr (i.e. this.value) which was incorrectly eliminated. It also does not account for the fact that the RHS of the assignment could cause side effects. Consider:

{
  int x = foo();
  return;
}

Even though x is unused, foo could cause side effects that we want to keep.

A canDiscard method has been added which is more conservative in deciding what can be discarded.

Result

After the fixes, the decompiled output is:

class Foo extends java.lang.Object {

    private int value;

    Foo() {
        super();
        return;
    }

    void increment(int parameterVar2) {
        Foo copyVar2_1;
        int insnVar4;
        int insnVar6;
        copyVar2_1 = this;
        insnVar4 = this.value;
        insnVar6 = copyVar2_1.value + parameterVar2;
        this.value = insnVar6;
        return;
    }
}

This is certainly more verbose, but it compiles and seems to be correct. This can be optimized in the future.

insnVar4 is unused because FieldAccessExpr is currently propagated like a constant. However, elimination keeps the original field access statement because field access may have side effects or throw an exception (e.g. trying to access a field an object that is null).

jade/src/main/kotlin/org/ucombinator/jade/decompile/Propogation.kt

Lines 199 to 221 in 844706a

	fun eval(expr: Expression, state: Map<String, LatticeValue>): LatticeValue =
	// Maps to a value in the lattice
	when (expr) {
	is IntegerLiteralExpr -> LatticeValue.Constant(expr)
	is NameExpr -> {
	if (phiVars.contains(expr.nameAsString)) {
	// assigns a phiVar which has an existing mapping
	// we don't want to propagate the value of a phiVar
	LatticeValue.Var(expr.nameAsString) // copy propagation
	} else {
	val value = state[expr.nameAsString]
	when (value) {
	is LatticeValue.Constant -> value
	is LatticeValue.Var -> value
	else -> LatticeValue.Var(expr.nameAsString) // wrap in a var, cld be a function
	}
	}
	}
	is StringLiteralExpr -> LatticeValue.Constant(expr)
	is FieldAccessExpr -> LatticeValue.Constant(expr)
	is ThisExpr -> LatticeValue.Constant(expr)
	else -> LatticeValue.Top
	}

Propagation should be updated so that field access is not copied freely. A later optimization could combine propagation and and elimination: if an expression has only one use and can be moved safely, inline it and remove its original assignment. This could also remove temporary variables such as copyVar2_1 and insnVar6.

Side note: Propogation needs to be renamed too. I'll rename it when refactoring.

Additional Info

This PR also introduces unit tests to verify behaviors and minimize risk of regressions. The test cases also clearly illustrate the expected behaviors.

Elimination was also refactored significantly to reduce code duplication and to improve readability. It has been reorganized into explicit graph-building, liveness-analysis, and pruning phases.