RFR JDK-8194978: Javac produces dead code for try-with-resource

[Jan Lahoda]

Hi Maurizio,

Thanks a lot for the comments. I've updated the patch according to the 
comments, updated webrev is here:
http://cr.openjdk.java.net/~jlahoda/8194978/webrev.01/

How does that look?

Thanks,
     Jan

On 20.3.2018 13:13, Maurizio Cimadamore wrote:
> Hi Jan,
> great work. The desugaring looks correct, at least there's nothing wrong
> I can spot about it. Some comments:
>
> * resourceNonNull = TreeInfo.skipParens(resource).hasTag(NEWCLASS);
>
> Is the above needed? I can't seem to support evidence that this syntax
> is even legal (in the branch where 'resource' is an expression, I mean)
>
> * The javadoc in Lower::makeTwrTry seems out of sync what is being
> actually generated
>
> * I think the Lower code would benefit from better variable names - the
> patch has a mixture of old names (e.g. primaryException) and new names
> (e.g. 'closeStatement2'). I suggest picking a naming scheme in the
> javadoc, and then stick to that in the code, so that there are explicit
> references to what bit it is that we're currently generating.
>
> * I find the code in Gen bit subtle; you want the new finally block to
> act as a finally - but only during the try block; that means, I believe,
> that you want genFinalizer to do the usual thing (e.g. copy finalizer
> body on exit points) but then skip doing that when it comes to
> exceptions. That is, you don't want a catch-all block and you don't want
> to copy finalizer body on exceptional paths. Another stacking would be
> to completely replace the 'env.info.finalize' after the body is done, e.g.
>
> <visit try body>
>
> if (tree.finalizer.flags & BODY_ONLY_FINALIZE != 0) {
>     env.info.finalize == null;
> }
>
> Wouldn't this be more consistent with what you are trying to achieve?
> Seems to me that with the current approach there are side effects that
> occur in the GenFinalizer even when afterBody is set (e.g. setting
> finalizer gaps in the encl environment).
>
> Cheers
> Maurizio
>
> On 15/03/18 15:40, Jan Lahoda wrote:
>> Hi,
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8194978
>>
>> The issue here is that the bytecode generated for try-with-resources
>> produces some unreachable instructions, and may be unnecessarily big.
>>
>> Consider this code:
>> ---
>>         try (InputStream in = new FileInputStream(str)) {
>>             int read = in.read();
>>             if (read == (-1)) return ;
>>             System.err.println(read);
>>         }
>> ---
>>
>> javac desugars it to this:
>> ---
>>         {
>>             final InputStream in = new FileInputStream(str);
>>             /*synthetic*/ Throwable primaryException0$ = null;
>>             try {
>>                 int read = in.read();
>>                 if (read == (-1)) return;
>>                 System.err.println(read);
>>             } catch (/*synthetic*/ final Throwable t$) {
>>                 primaryException0$ = t$;
>>                 throw t$;
>>             } finally {
>>                 if (primaryException0$ != null) try {
>>                     in.close();
>>                 } catch (/*synthetic*/ Throwable x2) {
>>                     primaryException0$.addSuppressed(x2);
>>                 } else in.close();
>>             }
>>         }
>> ---
>>
>> Which seems fine, but when the try-finally construct is written into
>> bytecode, the "finally" section is duplicated into all
>> (non-exceptional) exit points from the try body (it would also be
>> copied to non-exception exit points from all catches, but there are no
>> such exit points in this case). A rough idea on how the bytecode looks
>> like is below - please note that some try-catches don't embed nicely
>> with the rest of the code, so are denoted in comments only.
>>
>> ---
>>         {
>>             final InputStream in = new FileInputStream(str);
>>             /*synthetic*/ Throwable primaryException0$ = null;
>>             // try
>>                 int read = in.read();
>>                 if (read == (-1)) {
>>             // catch Throwable: goto CATCH
>>             // catch any: goto FINALLY [1]
>>                     //start injected finally
>>                     if (primaryException0$ != null) try { // [2]
>>                         in.close();
>>                     } catch (/*synthetic*/ Throwable x2) {
>>                         primaryException0$.addSuppressed(x2);
>>                     } else in.close();
>>                     //end injected finally
>>
>>                     return;
>>                 }
>>
>>             // try
>>                 System.err.println(read);
>>             // catch Throwable: goto CATCH
>>             // catch any: goto FINALLY [1]
>>
>>             //start injected finally
>>             if (primaryException0$ != null) try { // [2]
>>                 in.close();
>>             } catch (/*synthetic*/ Throwable x2) {
>>                 primaryException0$.addSuppressed(x2);
>>             } else in.close();
>>             //end injected finally
>>
>>             //goto END
>>
>>             CATCH: // catch (/*synthetic*/ final Throwable t$)
>>                 // try
>>                 primaryException0$ = t$;
>>                 throw t$;
>>                 // catch any: goto FINALLY
>>             FINALLY:
>>                 if (primaryException0$ != null) try {// [3]
>>                     in.close();
>>                 } catch (/*synthetic*/ Throwable x2) {
>>                     primaryException0$.addSuppressed(x2);
>>                 } else in.close();
>>             END:
>>         }
>> ---
>>
>> A few comments about the code:
>> -[1]: this catches seem unnecessary: all Throwables have already been
>> caught?
>> -[2]: at this place, it is clear that primaryException0$ is null, so
>> the is-non-null section is not useful
>> -[3]: at this place, it is mostly clear that primaryException0$ is
>> not-null, so the is-null section is not useful
>>
>> So, it seems it might be possible to construct the code so that only
>> the needed sections of the finally would be used at the appropriate
>> places. I.e. the original try-with-resources would get desugared to
>> something like:
>>
>> ---
>>         {
>>             final InputStream in = new FileInputStream(str);
>>             try {
>>                 int read = in.read();
>>                 if (read == (-1)) return;
>>                 System.err.println(read);
>>             } body-only-finally { //copied to exit points in try body
>>                                   //only, not used as a real finally
>>                 in.close();
>>             } catch (/*synthetic*/ final Throwable t$) {
>>                 try {
>>                     in.close();
>>                 } catch (/*synthetic*/ Throwable x2) {
>>                     t$.addSuppressed(x2);
>>                 }
>>                 throw t$;
>>             }
>>         }
>> ---
>>
>> Which would then get generated to bytecode as:
>> ---
>>         {
>>             final InputStream in = new FileInputStream(str);
>>             //try
>>                 int read = in.read();
>>                 if (read == (-1)) {
>>             // catch Throwable: goto CATCH
>>                     in.close();
>>                     return;
>>                 }
>>
>>             //try
>>                 System.err.println(read);
>>             // catch Throwable: goto CATCH
>>
>>             in.close();
>>
>>             //goto END
>>
>>             CATCH: // catch (/*synthetic*/ final Throwable t$)
>>                 try {
>>                     in.close();
>>                 } catch (/*synthetic*/ Throwable x2) {
>>                     t$.addSuppressed(x2);
>>                 }
>>                 throw t$;
>>             END:
>>         }
>> ---
>>
>> The current desugaring is trying to pull out the finally code into a
>> separate method when that appears to be worthwhile, but this proposed
>> desugaring seems to be producing a better bytecode with less javac code.
>>
>> Does this updated desugaring/transformation look safe?
>>
>> Webrev: http://cr.openjdk.java.net/~jlahoda/8194978/webrev.00/
>>
>> Thanks,
>>      Jan
>