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

[Maurizio Cimadamore]

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