Correct the logic to truncate Stack map in CodeContext.restoreLocalVariables

[maropu]

This PR proposes to correct the logic for truncating Stack map in CodeContext.restoreLocalVariables.
The current logic:

janino/janino/src/main/java/org/codehaus/janino/CodeContext.java

Lines 232 to 246 in 799ce44

	if (this.currentLocalScope != null) {
	StackMap sm = this.currentInserter.getStackMap();
	if (sm != null) {
	int numLocalsInStackMap = 0;
	for (VerificationTypeInfo slot : sm.locals()) {
	if (slot != StackMapTableAttribute.TOP_VARIABLE_INFO) {
	numLocalsInStackMap++;
	}
	}
	while (numLocalsInStackMap-- > this.allLocalVars.size()) {
	sm = sm.popLocal();
	}
	this.currentInserter.setStackMap(sm);
	}
	}

The proposed logic:

        // To truncate the stack map, it removes local variables
        // indicated by the popped scope.
        if (this.currentLocalScope != null) {
            StackMap sm = this.currentInserter.getStackMap();
            if (sm != null && sm.locals().length > 0) {
                int numActiveSlots = 0;
                int nextLvIndex = 0;
                for (VerificationTypeInfo slot : sm.locals()) {
                    if (nextLvIndex >= this.nextLocalVariableSlot) break;
                    nextLvIndex += slot.category();
                    numActiveSlots += 1;
                }
                int numRemovedSlots = sm.locals().length - numActiveSlots;
                while (numRemovedSlots-- > 0) sm = sm.popLocal();
                this.currentInserter.setStackMap(sm);
            }
        }

My previous fix in #146 causes another implicit bug and the Spark community found it in apache/spark#32716. The example code to reproduce it is as follows;

class MyClass {
  private boolean b1;

  public MyClass(boolean b1) {
    this.b1 = b1;
  }

   public Object apply(Object i) {
     final int value_0;
     // The bug still exist if the if condition is 'true'. Here we use a variable
     // to make the test more robust, in case the compiler can eliminate the else branch.
     if (b1) {
     } else {
       int field_0 = 1;
     }
     // The second if-else is necessary to trigger the bug.
     if (b1) {
     } else {
       // The bug disappear if it's an int variable.
       long field_1 = 2;
     }
     value_0 = 1;

     // The second final variable is necessary to trigger the bug.
     final int value_2;
     if (b1) {
     } else {
       int field_2 = 3;
     }
     value_2 = 2;

     return null;
   }
}

// When compiling the class above with janino, it throws an exception below;
[info]   Cause: java.lang.AssertionError:
[info]   at org.codehaus.janino.UnitCompiler.updateLocalVariableInCurrentStackMap(UnitCompiler.java:13227)
[info]   at org.codehaus.janino.UnitCompiler.store(UnitCompiler.java:12430)
[info]   at org.codehaus.janino.UnitCompiler.store(UnitCompiler.java:12406)
[info]   at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:2636)
[info]   at org.codehaus.janino.UnitCompiler.access$2700(UnitCompiler.java:231)
[info]   at org.codehaus.janino.UnitCompiler$6.visitLocalVariableDeclarationStatement(UnitCompiler.java:1539)
[info]   at org.codehaus.janino.UnitCompiler$6.visitLocalVariableDeclarationStatement(UnitCompiler.java:1523)
[info]   at org.codehaus.janino.Java$LocalVariableDeclarationStatement.accept(Java.java:3840)
[info]   at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:1523)
...

When truncating a stack map, the current logic fails to remove local variable slots referenced by a popped scope, so the assertion in UnitCompiler.java:13227 fails when reusing the slots.

janino/janino/src/main/java/org/codehaus/janino/UnitCompiler.java

Line 13227 in 799ce44

assert nextLvIndex <= lvIndex;

The proposed logic simply removes all local variables slots referenced by a popped scope (from scopeToPop.startingLocalVariableSlot to the end in locals). I added a new test for it and checked that all the existing tests can pass.

[maropu]

Could you review this? Thank you in advance @aunkrig @oontvoo

[kiszk]

Would it be possible to add more complicated test cases? For example,

Both paths have local variables with different levels.

if (b1) {
  double f0  = 1;
} else {
  float f0 = 1;
}

if (b1) {
  int f1 = 0;
} else {
  long f10 = 0;
}

if (b1) {
} else {
  int f2 = 0;
}

Multiple local variables in a block.

if (b1) {
  double f0  = 1;
} else {
  float f0 = 1;
}

if (b1) {
  int f10 = 0;
  long f11 = 0; 
  long f12 = 0; 
} else {
  long f10 = 0;
  int f11 = 0;
  int f12 = 0;
  long f13 = 0;
}

if (b1) {
  double f20 = 0;
  double f21 = 0; 
} else {
  float f20 = 0;
  float f21 = 0;
}

[maropu]

Adding more tests looks fine, but the tests above fail in the current master? I run a test with the code below;

        s.cook((""
                + "class MyClass {\n"
                + "    private boolean b1;\n"
                + "\n"
                + "    public MyClass(boolean b1) {\n"
                + "        this.b1 = b1;\n"
                + "    }\n"
                + "\n"
                + "    public Object func(Object i) {\n"
                + "        final int value_0;\n"
                + "        if (b1) {\n"
                + "            double f0  = 1;\n"
                + "        } else {\n"
                + "            float f0 = 1;\n"
                + "        }\n"
                + "\n"
                + "        if (b1) {\n"
                + "            int f1 = 0;\n"
                + "        } else {\n"
                + "            long f10 = 0;\n"
                + "        }\n"
                + "        value_0 = 1;\n"
                + "\n"
                + "        final int value_2;\n"
                + "        if (b1) {\n"
                + "        } else {\n"
                + "            int f2 = 0;\n"
                + "        }\n"
                + "        value_2 = 2;\n"
                + "\n"
                + "        return null;\n"
                + "    }\n"
                + "}\n"
                + "\n"
                + "public class JaninoTest {\n"
                + "    public static Object main() {\n"
                + "        MyClass c = new MyClass(true);\n"
                + "        return c.func(null);\n"
                + "    }\n"
                + "}\n"
        ));

[kiszk]

I did not run these tests in the current master.
Beyond that, I think that it would be good to increase the test coverage for future development. For example, in the current tests, only one variable exists in the block. What do you think?

[maropu]

Yea, adding more test cases is okay to me. But, if that is not directly related to the bug, I think its better to separate PRs.

[kiszk]

I would like to hear the opinions of others to add more tests in this PR or in a separate PR.

[maropu]

kindly ping @aunkrig @oontvoo

[aunkrig]

@maropu More unit tests in this field would be great; however I don't have much time these days. If you volunteer to provide a PR with more tests, I'd be grateful!

[kiszk]

@aunkrig Why do we need to close this PR? I think that your suggestion is to create another PR to prepare more tests.

I think that we can still work on this PR to resolve an issue.

[HyukjinKwon]

@aunkrig shall we reopen this PR for visibility? I thought this was merged.

[cloud-fan]

This PR provides a valid test that fails on the current master, can we move forward and adding more tests later? Downstream libraries are affected by this bug, and hopefully we can have a new janino release with this bug fixed.

[HyukjinKwon]

Hi @aunkrig can we reopen this or would you mind landing the fix please?

[shardulm94]

@aunkrig or @maropu Was this ultimately fixed or worked around? One of our users of https://github.com/apache/spark ran into #165 which is fixed now in Janino. But I see that Spark still uses 3.0.x deprecated line of Janino due to this issue. It would be great if Spark can be upgraded to Janino 3.1.x!

[aunkrig]

Hey Shardul,

this one completely passed me by... sorry for that, and thank you for the reminder!

I was able to reproduce the problem in the current version, and merged your fix into the HEAD, and verified that it doesn't break the regression test suite.

Please test!

[aunkrig]

To celebrate the day, I just published version 3.1.7 which contains this PR.

[shardulm94]

Thanks @aunkrig! Just to clarify, the PR was raised by @maropu and not me. I was able to test Janino v3.1.7 with the problematic test case mentioned here apache/spark#32716 and it passed.

@maropu @HyukjinKwon @cloud-fan Are we good to upgrade the Janino version in Spark now?

[cloud-fan]

Great! We can upgrade janino now :)

[aunkrig]

👏