RFR: 8302351: "assert(!JavaThread::current()->is_interp_only_mode() || !nm->method()->is_continuation_enter_intrinsic() || ContinuationEntry::is_interpreted_call(return_pc)) failed: interp_only_mode but not in enterSpecial interpreted entry" in fixup_callers_callsite

[Patricio Chilano Mateo]

On Wed, 24 May 2023 17:26:16 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Please review the following fix. Runtime methods called through the SharedRuntime::generate_resolve_blob() stub always return the value stored in _from_compiled_entry as the entry point to the callee method. This will either be the entry point to the compiled version of callee if there is one or the c2i adapter entry point. But this doesn't consider the case where an EnterInterpOnlyModeClosure handshake catches the JavaThread in the transition back to Java on those methods. In that case we should return the c2i adapter entry point even if there is a compiled entry point. Otherwise the JavaThread will continue calling the compiled versions of methods without noticing it's in interpreted only mode until it either calls a method that hasn't been compiled yet or it returns to the caller of that resolved callee where the change to interpreter only mode happened (since the EnterInterpOnlyModeClosure handshake marked all the frames on the stack for deoptimization). 
>> 
>> This is a long standing bug but has been made visible with the assert added as part of 8288949 where a related issue was fixed. There are more details in the bug comments about how this specific crash happens and its relation with 8288949. I also attached a reproducer.
>> 
>> These runtime methods are already using JRT_BLOCK_ENTRY/JRT_BLOCK so that the entry point to the callee is fetched only after the last possible safepoint in JRT_BLOCK_END. This guarantees that we will not return an entry point to compiled code that has already been removed. So the fix just adds a check to verify if the JavaThread entered into interpreted only mode in that transition back to Java and to return the c2i entry point instead.
>> 
>> I tested the patch in mach5 tiers 1-6. I also verified it with the reproducer I attached to the bug. I didn't include it as an additional test but I can do that if desired.
>> 
>> Thanks,
>> Patricio
>
> src/hotspot/share/runtime/sharedRuntime.cpp line 1507:
> 
>> 1505:   address target = current->is_interp_only_mode() ? callee_method->get_c2i_entry() : callee_method->verified_code_entry();
>> 1506:   assert(target != nullptr, "Jump to zero!");
>> 1507:   return target;
> 
> There are now 5 copies of this exact code block including this one.
> 
> Seems like this is screaming to be refactored into some static function
> with an appropriate name that can be called from all five places.

I considered that initially but I preferred the readability part given that we are not saving much. But I can do that if you think it's better that way.

> src/hotspot/share/runtime/sharedRuntime.cpp line 1627:
> 
>> 1625:     // interpreted version.
>> 1626:     return callee_method->get_c2i_entry();
>> 1627:   }
> 
> If I understand the analysis correctly, all this logic that we have to clearly
> identify the calling context as ` jdk.internal.vm.Continuation.enterSpecial`
> is no longer needed because we're switching to a more general fix of:
> 
>     if (current->is_interp_only_mode()) {
>       target = callee_method->get_c2i_entry();
>     } else {
>       target = callee_method->verified_code_entry();
>     }
>     return target;
> 
> And that more general fix cover the special case code that we're deleting
> (pun intended).

Exactly. This code was added to fix a related issue, where a JavaThread already noticed it was in interpreted only mode but because enterSpecial doesn't have an actual interpreted version we switched from interpreted to compiled and from there we incorrectly called the compiled version of enter() leading to asserts in JVMTI code about stack depth. This fix is more general so it already covers that case too.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/14108#discussion_r1204649529
PR Review Comment: https://git.openjdk.org/jdk/pull/14108#discussion_r1204650004