RFR: 8276836: Error in javac caused by switch expression without result expressions: Internal error: stack sim error

[Jan Lahoda]

On Thu, 11 Nov 2021 13:47:17 GMT, Guoxiong Li <gli at openjdk.org> wrote:

> Hi all,
> 
> The method `Gen#visitSwitchExpression` would switch code generation off by using the method `Code#markDead` when it meets the code like `throw new RuntimeException()`. Then the method `Gen#completeBinop` won't generate the expected code and wiil let the left operand stay in the stack without handling it.
> 
> This patch pops the left operand from the stack to solve this issue. And the corresponding test is added.
> 
> Thanks for taking the time to review.
> 
> Best Regards,
> -- Guoxiong

Thanks for looking into this and sorry for trouble.

One possible alternative that might work would be to put this only into `doHandleSwitchExpression`:

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
index 9ef735e1543..f5d6430eecb 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
@@ -1237,6 +1237,9 @@ public class Gen extends JCTree.Visitor {
             try {
                 handleSwitch(tree, tree.selector, tree.cases, tree.patternSwitch);
             } finally {
+                if (!code.isAlive()) {
+                    code.state.pop(code.state.stacksize);
+                }
                 code.setLetExprStackPos(prevLetExprStart);
             }
         } finally {


But I am not sure offhand if it would not have some problems.

But, I wonder if we need to fix the simulated stack at all (as it won't be completely sensible in anyway). Maybe all we need is to disable the assertion?

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
index 9ef735e1543..08b075822c6 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
@@ -971,7 +971,7 @@ public class Gen extends JCTree.Visitor {
                     genStat(tree.body, env);
                 }
 
-                if (code.state.stacksize != 0) {
+                if (code.state.stacksize != 0 && code.isAlive()) {
                     log.error(tree.body.pos(), Errors.StackSimError(tree.sym));
                     throw new AssertionError();
                 }


In general, I am afraid there needs to be more testing:

- tests for compound assignments, roughly the same as for binary operators
- longer chains on binary operators - e.g. `4 + switch () {...} + 5`, `4 + switch () {...} + switch () {...}`, etc. (three operands would probably be enough)
- various other operand types - method invocations, array dereferences
- tests that employ boolean conditions (inside an `if` condition, for example)
- the tests should not only compile the code - they should also run it, and verify the resulting behavior
- possibly use such switch that does not return 

I can help with writing some tests, if needed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6350