RFR: 8258032: Reconsider LEAF entry restrictions [v2]

[Daniel D.Daugherty]

On Tue, 5 Jan 2021 14:48:13 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change allows JRT_LEAF functions to create Handles because it doesn't need them but some code inside the VM needs Handles.  There's a NoSafepointVerifier in JRT_LEAF functions.
>> The JNI_LEAF and JVM_LEAF functions have NoHandleMark (so you can't create handles) but the transition ThreadInVMfromNative will reenable the HandleMarks, so that all this code doesn't have to do it itself.
>> Tested with tier1-6.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix copyrights.

So these changes boil down to:
- remove ResetNoHandleMark and NoHandleMark helpers in favor of having them in the appropriate interfaceSupport.inline.hpp macros and helpers
- ThreadInVMfromNative now has a ResetNoHandleMark helper
- VM_LEAF_BASE no longer needs a NoHandleMark helper
- VM_ENTRY_BASE_FROM_LEAF no longer needs a ResetNoHandleMark helper
- JNI_LEAF now has a NoHandleMark helper
- JVM_LEAF now has a NoHandleMark helper
- JVM_ENTRY_FROM_LEAF now has a ResetNoHandleMark helper

Okay, I think I grok these changes and they look good to me.
I would like to see Tier7 and Tier8 testing done since longer
stress runs are done in those tiers.

src/hotspot/share/c1/c1_Runtime1.cpp line 647:

> 645:   address pc = thread->exception_pc();
> 646:   // Still in Java mode
> 647:   nmethod* nm = NULL;

You are definitely going to want a C1 cognizant person to
review the changes in this file. The baseline's use of
ResetNoHandleMark and NoHandleMark in this file is an
absolute mess.

I don't understand the reason for old L647 here and for
old L652 below. Double ResetNoHandleMarks, but why?
I see no reason for old L647 so glad that it's gone!

src/hotspot/share/c1/c1_Runtime1.cpp line 1311:

> 1309:   patch_code(thread, load_klass_patching_id);
> 1310: 
> 1311:   // Back in JAVA, use no oops DON'T safepoint

The baseline here is even more confused.
debug_only NoHandleMark here on old L1311
and a ResetNoHandleMark on old L1315.
I see no reason for old L1311 so glad that it's gone!

src/hotspot/share/c1/c1_Runtime1.cpp line 1330:

> 1328: }
> 1329: 
> 1330: int Runtime1::move_appendix_patching(JavaThread* thread) {

And again a debug_only NoHandleMark on old L1330
and ResetNoHandleMark on old L1334. What the heck?

src/hotspot/share/c1/c1_Runtime1.cpp line 1349:

> 1347: // helper method which does the normal VM transition and when it
> 1348: // completes we can check for deoptimization. This simplifies the
> 1349: // assembly code in the cpu directories.

And again a debug_only NoHandleMark on old L1349
and ResetNoHandleMark on old L1353. What the heck?

src/hotspot/share/c1/c1_Runtime1.cpp line 1376:

> 1374:   // between the C calling convention and the Java one.
> 1375:   // e.g., on x86, GCC may clear only %al when returning a bool false, but
> 1376:   // JVM takes the whole %eax as the return value, which may misinterpret

And again a debug_only NoHandleMark on old L1376
and ResetNoHandleMark on old L1380. What the heck?

src/hotspot/share/jvmci/jvmciRuntime.cpp line 375:

> 373:   address pc = thread->exception_pc();
> 374:   // Still in Java mode
> 375:   CompiledMethod* cm = NULL;

I don't understand the reason for old L375 here and for
old L380 below. Double ResetNoHandleMarks, but why?
I see no reason for old L375 so glad that it's gone!

src/hotspot/share/opto/runtime.cpp line 1362:

> 1360:   SharedRuntime::_find_handler_ctr++;          // find exception handler
> 1361: #endif
> 1362:   nmethod* nm = NULL;

The baseline here is even more confused.
debug_only NoHandleMark here on old L1362
and a ResetNoHandleMark on old L1368.
I see no reason for old L1362 so glad that it's gone!

src/hotspot/share/runtime/deoptimization.cpp line 153:

> 151: 
> 152: // In order to make fetch_unroll_info work properly with escape
> 153: // analysis, The method was changed from JRT_LEAF to JRT_BLOCK_ENTRY.

nit - typo (not yours): s/, The/, the/

src/hotspot/share/runtime/handles.hpp line 292:

> 290: 
> 291: // ResetNoHandleMark is called in a context where there is an enclosing
> 292: // NoHandleMark.  Thread in _thread_in_native cannot create handles so

Perhaps:
// NoHandleMark. A thread in _thread_in_native must not create handles so

src/hotspot/share/runtime/handles.hpp line 293:

> 291: // ResetNoHandleMark is called in a context where there is an enclosing
> 292: // NoHandleMark.  Thread in _thread_in_native cannot create handles so
> 293: // this is used when transitioning via. ThreadInVMfromNative.

nit typo: s/via./via/

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

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1846