Re: Patch: Fix for PR java/4766

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Per" == Per Bothner <per@bothner.com> writes:

Tom> For PR java/4766:

Per> This is OK.

Thanks for looking at this so quickly.

Per> (1) This patch is basically an optimization.  If we just added a
Per> if (CAN_COMPLETE_NORMALLY (finally)) before the 'ret', would that
Per> work (though less efficiently)?  Otherwise, I'm wondering if we're
Per> just papering over some more subtle bug.

The generated code looks like this:

      0: getstatic #13=<Field java.lang.System.out java.io.PrintStream>
      3: ldc #15=<String "hi">
      5: invokevirtual #21=<Method java.io.PrintStream.println (java.lang.String)void>
      8: goto 16
     11: astore_0
     12: aload_0
     13: invokevirtual #27=<Method java.lang.Throwable.printStackTrace ()void>
     16: jsr 28
     19: goto 35
     22: astore_1
     23: jsr 28
     26: aload_1
     27: athrow
     28: astore_0
     29: iconst_0
     30: istore_2
     31: iload_2
     32: ireturn
     33: ret 0

The problem here is instruction 19, which is an invalid `goto'.
Removing that `goto' might be ok.  It isn't entirely clear to me.
Must the verifier process the `ret' in order to process the
instructions after the `jsr'?  The spec isn't really clear on this
point.

I changed the `goto' to 3 `nop's by editing the .class file.  gij and
gcj reject this code, but the JDK interpreter accepts it.

So I think the question is what to do after the `jsr' at the end of
the `try' block.  We can't emit a `return', since we don't have a
value to return (we could make one up, but that seems ugly).  We can't
emit a `goto' since there is no sensible target.

Per> (2) Does the compiler reject now reject the invalid bytecode (as
Per> generated by the old compiler) with a reasonable error message?
Per> Otherwise I wouldn't consider the PR fully fixed.

The compiler rejects it like so:

    creche. gcj --syntax-only PR4766.class
    PR4766.java: In class `PR4766':
    PR4766.java: In method `PR4766.myfunction()':
    PR4766.java:10: verification error at PC=26
    PR4766.java:10: stack overflow

That's not the message I was expecting (I would have expected
something about an invalid `goto'), but it makes sense.  I edited a
simpler piece of bytecode to have an invalid `goto'.  gcj correctly
detects that as well.  So, I think we're safe here.  I'm checking in
the patch.

Tom