RFR[13]: 8227260: Can't deal with SharedRuntime::handle_wrong_method triggering more than once for interpreter calls

[coleen.phillimore at oracle.com]

One comment

On 7/5/19 7:14 AM, Vladimir Ivanov wrote:
> Thanks for diagnosing the issue, Erik!
>
> Can you elaborate, please, on relation to clinit barrier in c2i?
>
> I don't see how it is possible to hit clinit barrier during i2c2i 
> transition. Template interpreter has clinit barrier as part of 
> invokestatic handler [1], so by the time c2i is reached, proper checks 
> should be already performed and all the conditions to pass the barrier 
> should be met.
>
> If SR::handle_wrong_method is called from clinit barrier in c2i during 
> i2c2i, it signals about a bug in the new clinit logic: somehow clinit 
> barrier is bypassed in interpreter.
>
> Are you sure it is not related to upcalls from native code 
> (caller_frame.is_entry_frame() [1])?
>
> Also, SR::handle_wrong_method() calls coming from clinit barriers 
> shouldn't hit the fast path w/ callee_target(), because it bypasses 
> the actual initialization check happening during call site re-resolution.
>
> Best regards,
> Vladimir Ivanov
>
> PS: regarding clearing JavaThread::_callee_target in 
> JavaThread::oops_do(), I'd prefer to keep it and limit the exposure of 
> a stale Method*. But it's just a matter of preference and I don't have 
> a strong opinion here.

The callee_method in JavaThread::oops_do() won't keep the Method* 
alive.  I'm not sure what keeps it alive in the callee_method field.   
Can there be a GC now with some Method* that you need there?

In that case, you should put the 
callee_method->method_holder()->java_mirror() in a new field in 
JavaThread::_callee_mirror or something, and have JavaThread::oops_do 
walk that.  Also, redefinition might have to keep the callee_method 
alive in the metadata walk, but you can file a separate bug for that if 
I'm not too confused.

Coleen

>
> [1]
> src/hotspot/share/runtime/sharedRuntime.cpp:
>
> JRT_BLOCK_ENTRY(address, 
> SharedRuntime::handle_wrong_method(JavaThread* thread))
> ...
>   if (caller_frame.is_interpreted_frame() ||
>       caller_frame.is_entry_frame()) {
>
>
> On 04/07/2019 18:02, Erik Österlund wrote:
>> Hi,
>>
>> The i2c adapter sets a thread-local "callee_target" Method*, which is 
>> caught (and cleared) by SharedRuntime::handle_wrong_method if the i2c 
>> call is "bad" (e.g. not_entrant). This error handler forwards 
>> execution to the callee c2i entry. If the 
>> SharedRuntime::handle_wrong_method method is called again due to the 
>> i2c2i call being still bad, then we will crash the VM in the 
>> following guarantee in SharedRuntime::handle_wrong_method:
>>
>> Method* callee = thread->callee_target();
>> guarantee(callee != NULL && callee->is_method(), "bad handshake");
>>
>> Unfortunately, the c2i entry can indeed fail again if it, e.g., hits 
>> the new class initialization entry barrier of the c2i adapter.
>> The solution is to simply not clear the thread-local "callee_target" 
>> after handling the first failure, as we can't really know there won't 
>> be another one. There is no reason to clear this value as nobody else 
>> reads it than the SharedRuntime::handle_wrong_method handler (and we 
>> really do want it to be able to read the value as many times as it 
>> takes until the call goes through). I found some confused clearing of 
>> this callee_target in JavaThread::oops_do(), with a comment saying 
>> this is a methodOop that we need to clear to make GC happy or 
>> something. Seems like old traces of perm gen. So I deleted that too.
>>
>> I caught this in ZGC where the timing window for hitting this issue 
>> seems to be wider due to concurrent code cache unloading. But it is 
>> equally problematic for all GCs.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8227260
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8227260/webrev.00/
>>
>> Thanks,
>> /Erik