Java 12: javac crash with switch expression containing try-catch-finally

[Jan Lahoda]

Hi Bernard,

I think I like this approach (possibly, it should also be used in the 
condition switch expr. branch). Do you want to take the patch to completion?

Thanks,
     Jan

On 26.3.2019 00:42, B. Blaser wrote:
> On Mon, 25 Mar 2019 at 19:33, Jan Lahoda <jan.lahoda at oracle.com> wrote:
>>
>> I've updated the patch with a version that strives to handle the boolean
>> switch expressions. Passes all tests except
>> tools/javac/switchexpr/CRT.java, which depends on the exact structure of
>> the generated code. Among other things, I'd like to see if we could
>> generate the current bytecode when there is no try-finally, which should
>> help with the CRT.java test. More testing still needed, though.
>>
>> Jan
>
> It seems we had the same problem with tests like
> 'switchexpr/ExpressionSwitchEmbedding.java'. You solved it with
> 'code.newRegSegment()' but you still allocate one temporary variable
> per 'break'.
>
> The following patch based on yours create only one temporary variable
> per switch expression and preserves roughly the same byte-code without
> 'try-finally' making tests like 'switchexpr/CRT.java' succeed.
>
> So, maybe you'll have an even better solution but 'langtools:tier1'
> along with the test you provided are OK.
>
> What do you think?
>
> Thanks,
> Bernard
>
> 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
> --- 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
> @@ -165,6 +165,7 @@
>       Chain switchExpressionTrueChain;
>       Chain switchExpressionFalseChain;
>       List<LocalItem> stackBeforeSwitchExpression;
> +    LocalItem switchResult;
>
>       /** Generate code to load an integer constant.
>        *  @param n     The integer to be loaded.
> @@ -1178,9 +1179,11 @@
>
>       private void doHandleSwitchExpression(JCSwitchExpression tree) {
>           List<LocalItem> prevStackBeforeSwitchExpression =
> stackBeforeSwitchExpression;
> +        LocalItem prevSwitchResult = switchResult;
>           int limit = code.nextreg;
>           try {
>               stackBeforeSwitchExpression = List.nil();
> +            switchResult = null;
>               if (hasTry(tree)) {
>                   //if the switch expression contains try-catch, the
> catch handlers need to have
>                   //an empty stack. So stash whole stack to local
> variables, and restore it before
> @@ -1199,6 +1202,7 @@
>                       stackBeforeSwitchExpression =
> stackBeforeSwitchExpression.prepend(item);
>                       item.store();
>                   }
> +                switchResult = makeTemp(tree.type);
>               }
>               int prevLetExprStart =
> code.setLetExprStackPos(code.state.stacksize);
>               try {
> @@ -1208,6 +1212,7 @@
>               }
>           } finally {
>               stackBeforeSwitchExpression = prevStackBeforeSwitchExpression;
> +            switchResult = prevSwitchResult;
>               code.endScopes(limit);
>           }
>       }
> @@ -1707,16 +1712,22 @@
>           Assert.check(code.isStatementStart());
>           final Env<GenContext> targetEnv;
>           if (tree.isValueBreak()) {
> -            //restore stack as it was before the switch expression:
> -            for (LocalItem li : stackBeforeSwitchExpression) {
> -                li.load();
> -            }
>               if (inCondSwitchExpression) {
>                   CondItem value = genCond(tree.value, CRT_FLOW_TARGET);
>                   Chain falseJumps = value.jumpFalse();
> -                targetEnv = unwindBreak(tree);
> +                Env<GenContext> localEnv = unwindBreak(tree);
>                   code.resolve(value.trueJumps);
> +                for (LocalItem li : stackBeforeSwitchExpression) {
> +                   li.load();
> +                }
>                   Chain trueJumps = code.branch(goto_);
> +                endFinalizerGaps(env, localEnv);
> +                code.resolve(falseJumps);
> +                targetEnv = unwindBreak(tree);
> +                for (LocalItem li : stackBeforeSwitchExpression) {
> +                    li.load();
> +                }
> +                falseJumps = code.branch(goto_);
>                   if (switchExpressionTrueChain == null) {
>                       switchExpressionTrueChain = trueJumps;
>                   } else {
> @@ -1731,9 +1742,17 @@
>                   }
>               } else {
>                   genExpr(tree.value, pt).load();
> -                code.state.forceStackTop(tree.target.type);
> +                if (switchResult != null) switchResult.store();
>                   targetEnv = unwindBreak(tree);
> -                targetEnv.info.addExit(code.branch(goto_));
> +                if (code.isAlive()) {
> +                    for (LocalItem li : stackBeforeSwitchExpression) {
> +                        li.load();
> +                    }
> +                    if (switchResult != null) switchResult.load();
> +                    code.state.forceStackTop(tree.target.type);
> +                    targetEnv.info.addExit(code.branch(goto_));
> +                    code.markDead();
> +                }
>               }
>           } else {
>               targetEnv = unwindBreak(tree);
>