review for 7055355: JSR 292: crash while throwing WrongMethodTypeException
Tom Rodriguez
tom.rodriguez at oracle.com
Thu Jun 16 10:18:44 PDT 2011
On Jun 16, 2011, at 9:24 AM, Christian Thalinger wrote:
> On Jun 16, 2011, at 4:44 PM, Christian Thalinger wrote:
>> 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?
I increased the slop value in my last set of changes and we're currently not failing this test that I can see. I agree that the test is slightly mystifying and wouldn't mind removing it.
>
> ...and there seems to be a bug in:
>
> void MethodHandles::verify_stack_move(MacroAssembler* _masm,
> RegisterOrConstant arg_slots, int direction) {
> enum { UNREASONABLE_STACK_MOVE = 256 * 4 }; // limit of 255 arguments
>
> We redefine UNREASONABLE_STACK_MOVE with a wrong value, it seems.
That's odd. I don't think I've even see this particular test fail so it's probably benign for the moment.
tom
>
> -- Christian
More information about the hotspot-compiler-dev
mailing list