RFR JDK-8220018: javac crash when compiling try-catch-finally inside switch expression

[Maurizio Cimadamore]

Hi,
I look at the code and discussed it offline with Jan extensively.

I think the approach looks good, and it is inspired by what we already 
do for return expressions (where we use intermediate locals to store 
return expressions in case there's a finalizer).

The only problem I have with this code is that it implicitly relies, in 
the conditional context case, on the fact that, should the finalizer 
complete abnormally (isAlive = false), then Code will refuse to emit any 
additional statements (such as the loads required to restore the stack 
state).

I think the is brittle, and might bite us back in case we revisit this 
code and add another statement whose behavior is NOT guarded by isAlive.

So, I suggest either adding explicit comments, saying that unwindBreak 
might affect isAlive and, as a result, following statements might not be 
generated - or, wrap the code in an explicit `if (isAlive)` guard, as 
done in the other branch of the visitBreak code. As bonus point, since 
we're restoring the stack in three different places, perhaps we can 
factor the restoring code out to some common function.

Cheers
Maurizio


On 09/05/2019 09:34, Jan Lahoda wrote:
> Hi Bernard,
>
> I apologize for late reply. Thanks for the patch, looks OK to me, but 
> I guess it would be good to have another review on this.
>
> Would it be possible to enhance the tests with the testcase below (and 
> any other testcases needed)?
>
> Thanks,
>     Jan
>
> On 27. 03. 19 19:20, B. Blaser wrote:
>> Hi,
>>
>> Please review the following fix for [1] which has already been
>> discussed in the corresponding thread [2]:
>>
>> http://cr.openjdk.java.net/~bsrbnd/jdk8220018/webrev.00/
>>
>> It includes the variant I suggested to store the switch expression
>> result in a single temporary variable along with Jan's solution for
>> switch expressions used as conditions. Note that I had to do a tiny
>> correction to the latter (see 'code.resolve(value.trueJumps)') to make
>> the following example succeed:
>>
>>      public static void test4() {
>>          if (!switch (0) {
>>              default -> {
>>                  try {
>>                      break switch(0) { default -> true; };
>>                  }
>>                  finally {
>>                      break false;
>>                  }
>>              }
>>          }) {
>>              System.out.println("OK!");
>>          }
>>      }
>>
>> It also includes Jan's test case.
>>
>> Any feedback is welcome (tier1 is OK).
>>
>> Thanks,
>> Bernard
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8220018
>> [2] 
>> http://mail.openjdk.java.net/pipermail/compiler-dev/2019-March/013073.html
>>