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

[Guoxiong Li]

On Mon, 29 Nov 2021 18:43:02 GMT, Jan Lahoda <jlahoda 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.

@lahodaj When I write the tests, I find that my patch fails at the test `4 + switch () {...} + switch () {...}`.

When the compiler handles the first switch expression, it pops the stack element so that the stack becomes empty. When the compiler handles the second switch expression, it pops the stack element just like the first one. But the stack is empty and an ArrayOutOfBoundException will be reported.

My current patch can't solve the issue. I will continue to find a better way to fix this bug. And the patch should be targeted to JDK19 because it seems not easy to solve it.

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

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