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

[B. Blaser]

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