review for 7055355: JSR 292: crash while throwing WrongMethodTypeException
Christian Thalinger
christian.thalinger at oracle.com
Thu Jun 16 05:49:19 PDT 2011
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
-- Christian
diff -r cfcf2ba8f3eb src/cpu/sparc/vm/methodHandles_sparc.cpp
--- a/src/cpu/sparc/vm/methodHandles_sparc.cpp
+++ b/src/cpu/sparc/vm/methodHandles_sparc.cpp
@@ -546,9 +546,9 @@ address MethodHandles::generate_method_h
__ cmp(O1_scratch, (int) vmIntrinsics::_invokeExact);
__ brx(Assembler::notEqual, false, Assembler::pt, invoke_generic_slow_path);
__ delayed()->nop();
- __ mov(O0_mtype, G5_method_type); // required by throw_WrongMethodType
+ __ mov(O0_mtype, G5_method_type); // required by throw_WrongMethodType
// mov(G3_method_handle, G3_method_handle); // already in this register
- __ jump_to(AddressLiteral(Interpreter::throw_WrongMethodType_entry()), O1_scratch);
+ __ jump_to(AddressLiteral(StubRoutines::throw_WrongMethodTypeException_entry()), O1_scratch);
__ delayed()->nop();
// here's where control starts out:
@@ -1142,26 +1142,14 @@ void MethodHandles::generate_method_hand
__ mov(O5_savedSP, SP); // Cut the stack back to where the caller started.
Label L_no_method;
- // FIXME: fill in _raise_exception_method with a suitable java.lang.invoke method
__ set(AddressLiteral((address) &_raise_exception_method), G5_method);
__ ld_ptr(Address(G5_method, 0), G5_method);
- __ tst(G5_method);
- __ brx(Assembler::zero, false, Assembler::pn, L_no_method);
- __ delayed()->nop();
const int jobject_oop_offset = 0;
- __ ld_ptr(Address(G5_method, jobject_oop_offset), G5_method);
- __ tst(G5_method);
- __ brx(Assembler::zero, false, Assembler::pn, L_no_method);
- __ delayed()->nop();
-
+ __ ld_ptr(Address(G5_method, jobject_oop_offset), G5_method); // dereference the jobject
__ verify_oop(G5_method);
__ jump_indirect_to(G5_method_fce, O3_scratch); // jump to compiled entry
__ delayed()->nop();
-
- // Do something that is at least causes a valid throw from the interpreter.
- __ bind(L_no_method);
- __ unimplemented("call throw_WrongMethodType_entry");
}
break;
diff -r cfcf2ba8f3eb src/cpu/sparc/vm/stubGenerator_sparc.cpp
--- a/src/cpu/sparc/vm/stubGenerator_sparc.cpp
+++ b/src/cpu/sparc/vm/stubGenerator_sparc.cpp
@@ -440,7 +440,8 @@ class StubGenerator: public StubCodeGene
#undef __
#define __ masm->
- address generate_throw_exception(const char* name, address runtime_entry, bool restore_saved_exception_pc) {
+ address generate_throw_exception(const char* name, address runtime_entry,
+ bool restore_saved_exception_pc, Register arg1 = noreg, Register arg2 = noreg) {
#ifdef ASSERT
int insts_size = VerifyThread ? 1 * K : 600;
#else
@@ -477,6 +478,13 @@ class StubGenerator: public StubCodeGene
if (VerifyThread) __ mov(G2_thread, O0); // about to be smashed; pass early
__ save_thread(noreg);
// do the call
+ if (arg1 != noreg) {
+ assert(arg2 != O1, "clobbered");
+ __ mov(arg1, O1);
+ }
+ if (arg2 != noreg) {
+ __ mov(arg2, O2);
+ }
BLOCK_COMMENT("call runtime_entry");
__ call(runtime_entry, relocInfo::runtime_call_type);
if (!VerifyThread)
@@ -3240,6 +3248,14 @@ class StubGenerator: public StubCodeGene
StubRoutines::_atomic_cmpxchg_long_entry = generate_atomic_cmpxchg_long();
StubRoutines::_atomic_add_ptr_entry = StubRoutines::_atomic_add_entry;
#endif // COMPILER2 !=> _LP64
+
+ // Build this early so it's available for the interpreter. The
+ // stub expects the required and actual type to already be in O1
+ // and O2 respectively.
+ StubRoutines::_throw_WrongMethodTypeException_entry =
+ generate_throw_exception("WrongMethodTypeException throw_exception",
+ CAST_FROM_FN_PTR(address, SharedRuntime::throw_WrongMethodTypeException),
+ false, G5_method_type, G3_method_handle);
}
More information about the hotspot-compiler-dev
mailing list