Request for reviews (L): 7012914: JSR 292 MethodHandlesTest C1: frame::verify_return_pc(return_address) failed: must be a return pc

[Christian Thalinger]

On Feb 21, 2011, at 8:05 PM, Tom Rodriguez wrote:
> 
> On Feb 21, 2011, at 9:15 AM, Christian Thalinger wrote:
> 
>> http://cr.openjdk.java.net/~twisti/7012914/webrev.01/
>> 
>> 7012914: JSR 292 MethodHandlesTest C1: frame::verify_return_pc(return_address) failed: must be a return pc
>> Reviewed-by:
>> 
>> The changes of 7007377 changed the way
>> MethodHandlesImpl.raiseException is called from the raise_exception
>> method handle adapter as it calls the C2I adapter.  Since
>> MethodHandlesImpl.raiseException has three arguments, on 32-bit x86 we
>> need to pass one argument on the stack.  This additional word on the
>> stack needs to be removed when we return to the actual callee during
>> unwinding.
>> 
>> Since the call site that triggers the exception is a method handle
>> call site we just need to restore the saved SP from BP.
>> 
>> Right now this is only a problem on 32-bit x86 because of the way we
>> implement the raise_exception method handle adapter.  But I assume
>> it's not impossible that in the future there are stack manipulations
>> before we throw an exception.  So a fix for all architectures seems
>> reasonable.
>> 
>> This change removes some unused code in
>> StubGenerator::generate_forward_exception as this stub is only used
>> for runtime calls with a pending exception and runtime calls can't be
>> method handle call sites.  It also removes the unused
>> ExceptionCache::_unwind_handler.
>> 
>> Additionally there is a small cleanup of
>> SharedRuntime::raw_exception_handler_for_return_address to not call
>> CodeCache::find_blob twice.
> 
> Isn't the second call unreachable?  If nm != null then we will already have returned.  So can't the whole block be replaced with:
> 
> guarantee(blob == NULL || !blob->is_runtime_stub(), "caller should have skipped stub");

You're right.  I changed that.

> 
> Otherwise it looks fine.

Thank you.

-- Christian