review for 7055355: JSR 292: crash while throwing WrongMethodTypeException

[Christian Thalinger]

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);
   }