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

[Jan Lahoda]

On Wed, 1 Dec 2021 16:13:30 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> 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.

Thanks @lgxbslgx, yes, I think this will be fairly complex, unfortunately. One more example (which does not include binary operators):

public class M {
     private void m(int i, int j) {
         m(0, switch (j) { default: if (true) throw new RuntimeException(); yield 0;} );
     }
}


Regarding disabling the assertion, OK, maybe that was too much. But we may have to do something more general than only a tweak for binary operators, unfortunately.

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

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