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

[Vladimir Ivanov]

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.

[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