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 19:30:08 GMT, Dean Long <dlong 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
>
> Do we ever call these resolve stubs on method handle intrinsics (if we could force the compilers not to inline them)?  I'm wondering what happens when we ask for the c2i adapter for intrinsics like _linkToNative, and also if these intrinsics do the right thing with is_interp_only_mode().  The fact that these intrinsics only have a compiled version came up before (JDK-8296336 and https://github.com/openjdk/jdk/pull/10933).  These intrinsics don't push a frame, but when they call the target do they check for is_interp_only_mode() and call the interpreted version instead of the compiled version?

@dean-long I added extra asserts in all these methods just for testing and these stubs are used to resolve method handle intrinsics too, including _linkToNative which I found from tests in test/jdk/java/foreign/. For the _linkToNative case which doesn't have an interpreted version if we return the c2i adapter, since the _i2i_entry is set to the i2c adapter [1] we will actually crash in the i2c because we cannot go from c2i -> i2c. Now if this is the only such case I guess we could special case it, assuming also the carrier's path to enterSpecial will never need to resolve that method. But I'm not convinced on that approach. Otherwise the other option would be to keep the current behavior and just remove the assert added in 8288949 accepting that it's too strong, and keep this issue under the existing 8218403 bug. What do you think?

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/method.cpp#L1488

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

PR Comment: https://git.openjdk.org/jdk/pull/14108#issuecomment-1564526499