Fix the bug that wrongly trims local vars in a stack #146

maropu · 2021-04-28T02:19:05Z

This PR intends to fix the bug where CodeContext#restoreLocalVariables wrongly trims local vars in a stack;

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

Lines 231 to 238 in f16db69

    
           // Truncate the stack map. 
        
           if (this.currentLocalScope != null) { 
        
               StackMap sm = this.currentInserter.getStackMap(); 
        
               if (sm != null) { 
        
                   while (sm.locals().length > this.allLocalVars.size()) sm = sm.popLocal(); 
        
                   this.currentInserter.setStackMap(sm); 
        
               } 
        
           }

The number of sm.locals() is not always the same with the number of allLocalVars because sm.locals() might contain the verification type TOP_VARIABLE_INFO. So, this PR excludes the type in sm.locals() when checking whether we could trim sm.locals() or not. Since the logic to trim sm.locals() was merged in the commit 66af9f2, this issue can happen in janino-master/v3.1.3. Note that janino does not trim sm.locals() before the commit, so janino-v3.1.2 and the earlier versions does not have the issue.

This PR adds a new test in ReportedBugsTest and the test throws an exception below without this fix;

Running org.codehaus.commons.compiler.tests.ReportedBugsTest
Tests run: 122, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 12.29 sec <<< FAILURE! - in org.codehaus.commons.compiler.tests.ReportedBugsTest
testPullRequestXX[CompilerFactory=janino](org.codehaus.commons.compiler.tests.ReportedBugsTest)  Time elapsed: 0.012 sec  <<< ERROR!
org.codehaus.commons.compiler.InternalCompilerException: Compiling "JaninoTest" in Line 1, Column 8: Line 2, Column 16: Compiling "func(int v)": Line 20, Column 37: Compiling "(local_var0 + local_var3) % 2": Line 20, Column 14: Compiling "(local_var0 + local_var3)": Line 20, Column 27: Compiling "local_var3": Invalid local variable index 8
	at org.codehaus.commons.compiler.tests.ReportedBugsTest.testPullRequestXX(ReportedBugsTest.java:1225)
Caused by: org.codehaus.commons.compiler.InternalCompilerException: Line 2, Column 16: Compiling "func(int v)": Line 20, Column 37: Compiling "(local_var0 + local_var3) % 2": Line 20, Column 14: Compiling "(local_var0 + local_var3)": Line 20, Column 27: Compiling "local_var3": Invalid local variable index 8
	at org.codehaus.commons.compiler.tests.ReportedBugsTest.testPullRequestXX(ReportedBugsTest.java:1225)
Caused by: org.codehaus.commons.compiler.InternalCompilerException: Line 20, Column 37: Compiling "(local_var0 + local_var3) % 2": Line 20, Column 14: Compiling "(local_var0 + local_var3)": Line 20, Column 27: Compiling "local_var3": Invalid local variable index 8
	at org.codehaus.commons.compiler.tests.ReportedBugsTest.testPullRequestXX(ReportedBugsTest.java:1225)
Caused by: org.codehaus.commons.compiler.InternalCompilerException: Line 20, Column 14: Compiling "(local_var0 + local_var3)": Line 20, Column 27: Compiling "local_var3": Invalid local variable index 8
	at org.codehaus.commons.compiler.tests.ReportedBugsTest.testPullRequestXX(ReportedBugsTest.java:1225)
Caused by: org.codehaus.commons.compiler.InternalCompilerException: Line 20, Column 27: Compiling "local_var3": Invalid local variable index 8
	at org.codehaus.commons.compiler.tests.ReportedBugsTest.testPullRequestXX(ReportedBugsTest.java:1225)
Caused by: org.codehaus.commons.compiler.InternalCompilerException: Invalid local variable index 8
	at org.codehaus.commons.compiler.tests.ReportedBugsTest.testPullRequestXX(ReportedBugsTest.java:1225)

The code tries to reference the already-trimmed local var in a stack then it fails. I've checked that all the existing tests can pass with this fix merged.

SIDE NOTE: I found this bug when checking whether Spark can land on the latest janino. For example, a query below fails when using spark-master + janino-master/v3.1.3;

Spark session available as 'spark'.
Welcome to
      ____              __
     / __/__  ___ _____/ /__
    _\ \/ _ \/ _ `/ __/  '_/
   /___/ .__/\_,_/_/ /_/\_\   version 3.2.0-SNAPSHOT
      /_/
         
Using Scala version 2.12.10 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_181)
Type in expressions to have them evaluated.
Type :help for more information.

scala> val df = Seq(("2016-03-27 19:39:34", 1, "a"), ("2016-03-27 19:39:56", 2, "a"), ("2016-03-27 19:39:27", 4, "b")).toDF("time", "value", "id")
scala> val rdf = df.select(window($"time", "10 seconds", "3 seconds", "0 second"), $"value").orderBy($"window.start".asc, $"value".desc).select("value")
scala> rdf.show()
21/04/26 23:11:07 ERROR CodeGenerator: failed to compile: org.codehaus.commons.compiler.InternalCompilerException: Compiling "GeneratedClass" in File 'generated.java', Line 1, Column 1: File 'generated.java', Line 32, Column 16: Compiling "processNext()": File 'generated.java', Line 2010, Column 6: Compiling "filter_isNull_3": Invalid local variable index 249
org.codehaus.commons.compiler.InternalCompilerException: Compiling "GeneratedClass" in File 'generated.java', Line 1, Column 1: File 'generated.java', Line 32, Column 16: Compiling "processNext()": File 'generated.java', Line 2010, Column 6: Compiling "filter_isNull_3": Invalid local variable index 249
	at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:369)
	at org.codehaus.janino.UnitCompiler.access$000(UnitCompiler.java:231)
	at org.codehaus.janino.UnitCompiler$1.visitCompilationUnit(UnitCompiler.java:333)
	at org.codehaus.janino.UnitCompiler$1.visitCompilationUnit(UnitCompiler.java:330)
	at org.codehaus.janino.Java$CompilationUnit.accept(Java.java:367)
	at org.codehaus.janino.UnitCompiler.compileUnit(UnitCompiler.java:330)
	at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:245)
	at org.codehaus.janino.ClassBodyEvaluator.cook(ClassBodyEvaluator.java:294)
	at org.codehaus.janino.ClassBodyEvaluator.cook(ClassBodyEvaluator.java:288)
	at org.codehaus.janino.ClassBodyEvaluator.cook(ClassBodyEvaluator.java:267)
	at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:82)
	at org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator$.org$apache$spark$sql$catalyst$expressions$codegen$CodeGenerator$$doCompile(CodeGenerator.scala:1403)
	at org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator$$anon$1.load(CodeGenerator.scala:1501)
	at org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator$$anon$1.load(CodeGenerator.scala:1498)
    ...

See this link for the fully-generated code by Spark.

kiszk · 2021-04-28T09:18:41Z

Could you please elaborate on why we need to remove only Top_variable_info in verification_type_info at https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.4

maropu · 2021-04-28T12:11:11Z

yea, I've checked the section, the 4.10.1.2. Verification Type System section, and the janino implementation before opening this PR. IIUC the other verification types seem to have coresponding local vars in allLocalVars. On the other hand, only the TOP_VARIABLE_INFO does not have coresponding local vars there and it seems it is used to represent already-used indices in UnitCompiler#updateLocalVariableInCurrentStackMap:

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

Lines 13226 to 13230 in f16db69

    
           while (nextLvIndex < lvIndex) { 
        
               ci.setStackMap(ci.getStackMap().pushLocal(StackMapTableAttribute.TOP_VARIABLE_INFO)); 
        
               nextLvIndex++; 
        
           } 
        
           ci.setStackMap(ci.getStackMap().pushLocal(vti));

maropu · 2021-05-05T03:53:19Z

ping @oontvoo @aunkrig

aunkrig · 2021-05-06T09:37:25Z

@maropu I just merged your fix. As for #145, thank you for the excellent work!

maropu · 2021-05-06T10:00:11Z

Thank you for the check, @aunkrig !

aunkrig · 2021-05-06T12:16:53Z

Janino 3.1.4 is just out the door.

### What changes were proposed in this pull request? This PR proposes to bump up the janino version from 3.0.16 to v3.1.4. The major changes of this upgrade are as follows: - Fixed issue #131: Janino 3.1.2 is 10x slower than 3.0.11: The Compiler's IClassLoader was initialized way too eagerly, thus lots of classes were loaded from the class path, which is very slow. - Improved the encoding of stack map frames according to JVMS11 4.7.4: Previously, only "full_frame"s were generated. - Fixed issue #107: Janino requires "org.codehaus.commons.compiler.io", but commons-compiler does not export this package - Fixed the promotion of the array access index expression (see JLS7 15.13 Array Access Expressions). For all the changes, please see the change log: http://janino-compiler.github.io/janino/changelog.html NOTE1: I've checked that there is no obvious performance regression. For all the data, see a link: https://docs.google.com/spreadsheets/d/1srxT9CioGQg1fLKM3Uo8z1sTzgCsMj4pg6JzpdcG6VU/edit?usp=sharing NOTE2: We upgraded janino to 3.1.2 (#27860) once before, but the commit had been reverted in #29495 because of the correctness issue. Recently, #32374 had checked if Spark could land on v3.1.3 or not, but a new bug was found there. These known issues has been fixed in v3.1.4 by following PRs: - janino-compiler/janino#145 - janino-compiler/janino#146 ### Why are the changes needed? janino v3.0.X is no longer maintained. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? GA passed. Closes #32455 from maropu/janino_v3.1.4. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Sean Owen <[email protected]>

maropu added 2 commits April 28, 2021 10:46

Fix the bug that wrongly trims local vars in a stack

3543d67

Add the PR number in the test name

801f9d8

maropu mentioned this pull request Apr 28, 2021

Fix the bug that handles NaN comparisons incorrectly #145

Merged

maropu mentioned this pull request Apr 30, 2021

[WIP][SPARK-35253][BUILD][SQL] Upgrade Janino from 3.0.16 to 3.1.3 apache/spark#32374

Closed

aunkrig merged commit d6bc53c into janino-compiler:master May 6, 2021

maropu mentioned this pull request May 6, 2021

[SPARK-35253][SQL][BUILD] Bump up the janino version to v3.1.4 apache/spark#32455

Closed

maropu mentioned this pull request Jun 2, 2021

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the bug that wrongly trims local vars in a stack #146

Fix the bug that wrongly trims local vars in a stack #146

maropu commented Apr 28, 2021

kiszk commented Apr 28, 2021

maropu commented Apr 28, 2021

maropu commented May 5, 2021 •

edited

Loading

aunkrig commented May 6, 2021

maropu commented May 6, 2021

aunkrig commented May 6, 2021

	// Truncate the stack map.
	if (this.currentLocalScope != null) {
	StackMap sm = this.currentInserter.getStackMap();
	if (sm != null) {
	while (sm.locals().length > this.allLocalVars.size()) sm = sm.popLocal();
	this.currentInserter.setStackMap(sm);
	}
	}

Fix the bug that wrongly trims local vars in a stack #146

Fix the bug that wrongly trims local vars in a stack #146

Conversation

maropu commented Apr 28, 2021

kiszk commented Apr 28, 2021

maropu commented Apr 28, 2021

maropu commented May 5, 2021 • edited Loading

aunkrig commented May 6, 2021

maropu commented May 6, 2021

aunkrig commented May 6, 2021

maropu commented May 5, 2021 •

edited

Loading