RFR (XS) 8222297: IRT_ENTRY/IRT_LEAF etc are the same as JRT

[David Holmes]

Hi Coleen,

I was busy doing some archaeology on this code so didn't notice the RFR. 
Glad Dan picked up on the only difference with the "verifiers" in the 
LEAF variants.

FTR the differences here are historical. JRT was added first and shortly 
needed to manifest the current thread directly. IRT were added later and 
the "thread" was an implicit argument. But by July 1998 the two ENTRY 
macros were the same. The only difference was the verifier in the LEAF, 
and some custom variants of each macro that no longer exist.

Conceptually I've always thought there was a difference in how the 
interpreter needed to "enter the runtime" versus the compilers. So the 
different macros made that clear. But if the requirements are 
essentially identical, and always have been, then the distinction just 
confuses things.

I'm not clear what your resolution is here? Just accept that maybe we 
lost a verify-gc call as Dan noted?

Thanks,
David

On 12/04/2019 7:06 am, coleen.phillimore at oracle.com wrote:
> 
> Dan, Thank you for reviewing.
> 
> On 4/11/19 3:02 PM, Daniel D. Daugherty wrote:
>> On 4/11/19 11:39 AM, coleen.phillimore at oracle.com wrote:
>>> Summary: Replace IRT entry points with JRT.
>>>
>>> Tested with hs tier1-3 and built zero.  And grepped from the right 
>>> level directory this time.
>>>
>>> open webrev at 
>>> http://cr.openjdk.java.net/~coleenp/2019/8222297.01/webrev
>>
>> src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp
>>     No comments.
>>
>> src/hotspot/cpu/arm/interpreterRT_arm.cpp
>>     No comments.
>>
>> src/hotspot/cpu/ppc/interpreterRT_ppc.cpp
>>     No comments.
>>
>> src/hotspot/cpu/s390/interpreterRT_s390.cpp
>>     No comments.
>>
>> src/hotspot/cpu/sparc/interpreterRT_sparc.cpp
>>     No comments.
>>
>> src/hotspot/cpu/x86/interpreterRT_x86_32.cpp
>>     No comments.
>>
>> src/hotspot/cpu/x86/interpreterRT_x86_64.cpp
>>     No comments.
>>
>> src/hotspot/cpu/zero/cppInterpreter_zero.cpp
>>     No comments.
>>
>> src/hotspot/cpu/zero/interpreterRT_zero.cpp
>>     No comments.
>>
>> src/hotspot/share/interpreter/interpreterRuntime.cpp
>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>     old L435: #define IRT_LEAF(result_type, header)
>>     old L438:     debug_only(NoSafepointVerifier __nspv(true);)
>>     new L432: #define JRT_LEAF(result_type, header)
>>     new L435:   debug_only(JRTLeafVerifier __jlv;)
>>         src/hotspot/share/runtime/interfaceSupport.cpp:
>>
>>           JRTLeafVerifier::JRTLeafVerifier()
>>             : NoSafepointVerifier(true, 
>> JRTLeafVerifier::should_verify_GC())
>>           {
>>           }
>>
>>         src/hotspot/share/runtime/safepointVerifiers.hpp:
>>
>>           NoSafepointVerifier(bool activated = true, bool verifygc = 
>> true ) :
>>             NoGCVerifier(verifygc),
>>             _activated(activated) {
>>
>>         IRT_LEAF creates a NoSafepointVerifier with first ctr param == 
>> true
>>         and the second ctr param == default true.
>>
>>         JRT_LEAF creates a JRTLeafVerifier subclassed on 
>> NoSafepointVerifier
>>         with first ctr param == true and second ctr param based on
>>         JRTLeafVerifier::should_verify_GC() which can return either
>>         true or false depending on the calling thread's state. If the
>>         thread's state == _thread_in_Java, then the return == true.
>>         If the thread's state == _thread_in_native, then the return == 
>> false.
>>
>>         As long as all the IRT_LEAF uses are thread state == 
>> _thread_in_Java
>>         then this is an equivalent change.
>>
>>         I found these uses of IRT_LEAF:
>>
>>           SharedRuntime::fixup_callers_callsite()
> 
> This is called from the c2i adapter so would be thread_in_java.
> 
>>     InterpreterRuntime::bcp_to_di()
>>           InterpreterRuntime::verify_mdp()
>>           InterpreterRuntime::interpreter_contains()
>>           InterpreterRuntime::popframe_move_outgoing_args()
>>           InterpreterRuntime::trace_bytecode()
>>
> 
> I assume these are thread_in_java too, since they have access to 
> metadata (except interpreter_contains, whose callers have access to 
> metadata).  This would be dangerous for in_native threads to touch 
> metadata directly.
> 
>> I have not checked to see if these IRT_LEAF functions are
>>         ever called from thread state == _thread_in_native locations,
>>         but if they are, then we will no longer 'verifygc' with the
>>         JRT_LEAF switch.
>>
> It seems like if one of these IRT calls was from native the
> 
> NoSafepointVerifier( true /* no safepoints */, false /* don't verify GC 
> */) might be a fix to these methods.  Because of the comment:
> 
>    case _thread_in_native:
>      // A native thread is not subject to safepoints.
>      // Even while it is in a leaf routine, GC is ok
>      return false;
> 
> But I think it's the case that the behavior is the same, or more 
> consistent if one of these is a native LEAF call that had IRT_LEAF. 
> Trying to resolve these subtle differences is the trouble with mostly 
> duplicated code :(
> 
> Thanks!
> Coleen
> 
>> src/hotspot/share/runtime/sharedRuntime.cpp
>>     No comments.
>>
>>
>> Your call on what to do about the difference that I found between
>> IRT_LEAF and JRT_LEAF. We could be losing a 'verifygc' check here,
>> but...
>>
>> Dan
>>
>>
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8222297
>>>
>>> Thanks,
>>> Coleen
>>
>