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

[Daniel D. Daugherty]

On Tue, 23 May 2023 22:07:37 GMT, Patricio Chilano Mateo <pchilanomate 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

Thumbs up on the fix itself. I do think it could use some refactoring
to avoid the five copies of the same code block.

Thanks for documenting your testing. So far there have been 15 sightings
of this test failure in the JDK21 CI: 13 in tier4, 2 in tier7 and 1 in tier8. Since
you are touching 5 different SharedRuntime functions, I think you should
run this fix thru at least one Tier7 and one Tier8 job set.

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.

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).

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

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14108#pullrequestreview-1442425060
PR Review Comment: https://git.openjdk.org/jdk/pull/14108#discussion_r1204544067
PR Review Comment: https://git.openjdk.org/jdk/pull/14108#discussion_r1204550313