review for 7055355: JSR 292: crash while throwing WrongMethodTypeException

[Christian Thalinger]

On Jun 16, 2011, at 2:49 PM, Christian Thalinger wrote:
> On Jun 16, 2011, at 11:47 AM, Christian Thalinger wrote:
>> On Jun 16, 2011, at 7:32 AM, Tom Rodriguez wrote:
>>> http://cr.openjdk.java.net/~never/7055355
>>> 174 lines changed: 54 ins; 111 del; 9 mod; 30934 unchg
>>> 
>>> 7055355: JSR 292: crash while throwing WrongMethodTypeException
>>> Reviewed-by:
>>> 
>>> When throwing a WrongMethodTypeException from a MethodHandle the
>>> existing code was pretending it was in the interpreter which worked ok
>>> most of the time but would sometimes die when trying to throw with a
>>> compiled caller.  It could cause asserts in some contexts or result in
>>> GC crashes.  The fix is to remove the old machinery in favor of the
>>> more standard code for throwing exceptions in
>>> StubRoutines/SharedRuntime.  There was one other use of the old throw
>>> machinery but it's in part of check that can never fail since we won't
>>> enable method handles unless we have a non-NULL rasie exception
>>> method.  There's an extra fix in the there to reject lookups of
>>> clinit.  It isn't critical but it allows all the JCK tests to run
>>> without asserting.  Tested with failing JCK test and jruby crasher on
>>> sparc/x86 with client/server/d64, along with a full run of the
>>> invokedynamic JCK tests, the JLI regression tests and the vm.mlvm
>>> tests.
>> 
>> You didn't remove the NULL-checking code of the raise_exception method in methodHandles_sparc.cpp.  Otherwise this looks good.
> 
> 
> The SPARC logic is broken, I get asserts with John's ThrowExceptionsTest.  The problem is that SPARC's throw exception stub uses a save_frame and O1/O2 then contain some arbitrary values hitting the assert in SharedRuntime::throw_WrongMethodTypeException.  I fixed it using the same logic you added for x86.  I also removed the NULL-checking code I mentioned above.
> 
> With the patch below applied we hit the unreasonable-stack-move assert at some point:
> 
> #  assert(false) failed: DEBUG MESSAGE: damaged ricochet frame: (L4 - UNREASONABLE_STACK_MOVE) > FP

I know I ported this piece of code over from x86 but honestly I don't understand what it is checking for.  L4 >= FP seems reasonable and we should keep that check but (L4 - UNREASONABLE_STACK_MOVE) > FP is not a constraint that must hold in all situations.  I suggest removing that check.  What do you think?

-- Christian