search

janino-compiler / janino

Janino is a super-small, super-fast Java™ compiler.
http://janino-compiler.github.io/janino
Other
1.25k stars 208 forks source link

Correct the logic to truncate Stack map in CodeContext.restoreLocalVariables #148

Closed maropu closed 2 years ago

maropu commented 3 years ago

This PR proposes to correct the logic for truncating Stack map in CodeContext.restoreLocalVariables. The current logic: https://github.com/janino-compiler/janino/blob/799ce445bbd81599b1f7fdafcb47049cae40e22d/janino/src/main/java/org/codehaus/janino/CodeContext.java#L232-L246

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 https://github.com/apache/spark/pull/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.

https://github.com/janino-compiler/janino/blob/799ce445bbd81599b1f7fdafcb47049cae40e22d/janino/src/main/java/org/codehaus/janino/UnitCompiler.java#L13227

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 commented 3 years ago

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

kiszk commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

maropu commented 3 years ago

kindly ping @aunkrig @oontvoo

aunkrig commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

cloud-fan commented 3 years ago

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 commented 3 years ago

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

shardulm94 commented 2 years ago

@aunkrig or @maropu Was this ultimately fixed or worked around? One of our users of https://github.com/apache/spark ran into https://github.com/janino-compiler/janino/issues/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 commented 2 years ago

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 commented 2 years ago

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

shardulm94 commented 2 years ago

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 https://github.com/apache/spark/pull/32716 and it passed.

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

cloud-fan commented 2 years ago

Great! We can upgrade janino now :)

aunkrig commented 2 years ago

:clap: