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

[B. Blaser]

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