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

[Jan Lahoda]

Hi Bernard,

Looks good to me. Any comments, Maurizio?

Thanks,
     Jan

On 19. 05. 19 16:46, B. Blaser wrote:
> Hi Jan and Maurizio,
> 
> I've updated the webrev [1] as you both suggested:
> * 'unwindBreak()' commented to warn for a possible dead code marking.
> * 'reloadStackBeforeSwitchExpr()' added to factorize the stack reloading.
> * 'test4()' below added to Jan's test cases.
> 
> Does this look good to both of you (tier1 is OK)?
> 
> Thanks,
> Bernard
> 
> [1] http://cr.openjdk.java.net/~bsrbnd/jdk8220018/webrev.01/
> 
> 
> On Sat, 11 May 2019 at 13:29, B. Blaser <bsrbnd at gmail.com> wrote:
>>
>> Hi Jan and Maurizio,
>>
>> Thanks for both feedback.
>>
>> On Thu, 9 May 2019 at 14:35, Maurizio Cimadamore
>> <maurizio.cimadamore at oracle.com> wrote:
>>>
>>> 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 first step, I guess I'd be more for simply adding a comment but we
>> can still rework and consolidate this part later on if necessary.
>>
>>> 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.
>>
>> Of course ;-)
>>
>>> Cheers
>>> Maurizio
>>>
>>>
>>> On 09/05/2019 09:34, Jan Lahoda wrote:
>>>> Hi Bernard,
>>>>
>>>> I apologize for late reply.
>>
>> No problem, I'm rather busy too...
>>
>>>> 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)?
>>
>> Yes, I'll also include this one.
>>
>> Cheers,
>> Bernard
>>
>>
>>>> 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
>>>>>