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

Fri Apr 12 11:39:41 UTC 2019

On 4/11/19 7:24 PM, David Holmes wrote:
> 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.

Right.  And as I put it in the bug, there have been some extensions to 
the JRT entries that every now and then, I think I need for the 
interpreter, so these distinctions are just confusing.
>
> I'm not clear what your resolution is here? Just accept that maybe we 
> lost a verify-gc call as Dan noted?

I think there is no actual lost verify call, in that there is no IRT 
entry coming from native (that I can spot visually without running 
verification), and I can't figure why out it's makes sense to verify 
that there's no GC, when we've already verified that there is no safepoint.

NoGCVerifier::NoGCVerifier(bool verifygc) {
   _verifygc = verifygc;
   if (_verifygc) {
     CollectedHeap* h = Universe::heap();
     assert(!h->is_gc_active(), "GC active during NoGCVerifier");
     _old_invocations = h->total_collections();
   }

When is_gc_active is set here:

void GenCollectedHeap::do_collection(bool           full,
...
   assert(SafepointSynchronize::is_at_safepoint(), "should be at 
safepoint");
...
   FlagSetting fl(_is_gc_active, true);

Except for ZGC, which I can't tell, increment_total_collections also is 
called at a safepoint.

It might be a useful assert if we want to prevent checking for the start 
of a concurrent collection.  If the thread is in native, it doesn't 
actually make sense because in native should not have any direct access 
to oops or metadata.  So if the verification is actually lost, which I 
doubt, that's a good thing.

Coleen

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