RFR: 8258032: Reconsider LEAF entry restrictions

[David Holmes]

On 8/01/2021 7:46 am, Daniel D.Daugherty wrote:
> On Thu, 7 Jan 2021 20:57:00 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:
> 
>> Please review the smaller version of this enhancement.  JRT_LEAF can allow Handles.  Fixed places where a ResetNoHandleMark was added to workaround the restriction and remove some obsolete comments in deoptimization (the function is a JRT_BLOCK_ENTRY that has a HandleMarkCleaner which cleans up the HandleMark to the one before call_stub() in JavaCalls. A HandleMarkCleaner also allows HandleMark and Handles.
>>
>> Tested with tier1-3, 4-6 with the larger change and 7-8 in progress.
> 
> Like the original changes, the most interesting stuff is in
> interfaceSupport.inline.hpp. Just to keep it clear in my head:
> - remove NoHandleMark helper from VM_LEAF_BASE; it gets added to JNI_LEAF and JVM_LEAF which need it; JRT_LEAF doesn't need it.

If I read the bug comments correctly it is more a case of:

"JRT_LEAF should also need it, because this is a leaf routine and we 
shouldn't be messing with handles, but we have code that wants to mess 
with Handles even from leaf routines, and that is safe so let's allow 
them to do that."

This strikes me as totally butchering what we mean by a "leaf" routine. :(

I also can't quite see if Deoptimization::fetch_unroll_info can actually 
be reverted back to a JRT_LEAF with these proposed changes?

> - remove ResetNoHandleMark helper from VM_ENTRY_BASE_FROM_LEAF since ThreadInVMfromNative will have one with your fix for JDK-8259374.

Can we add a comment describing what ResetNoHandleMark actually does 
please. I'm not really understanding how it is supposed to be used. I 
suspect it is to allow code that should not create Handles to call code 
that will create Handles without having to introduce convoluted scoping 
for  the NHM.

Thanks,
David
-----

> Please see my comment in JVM_ENTRY_FROM_LEAF.
> 
> src/hotspot/share/gc/z/zVerify.cpp line 376:
> 
>> 374: // is no safepoint to protect against, and fiddling around with exceptions.
>> 375: class StackWatermarkProcessingMark {
>> 376:   HandleMark            _hm;
> 
> Since you're removing the ResetNoHandleMark, you should revisit
> the comment block for the class. The entire sentence to revisit is:
> // ... It is mostly
> // due to problems that we might want to eventually clean up inside of the
> // frame iteration code, such as creating random handles even though there
> // is no safepoint to protect against, and fiddling around with exceptions.
> 
> src/hotspot/share/runtime/stackWatermark.cpp line 80:
> 
>> 78: // is no safepoint to protect against, and fiddling around with exceptions.
>> 79: class StackWatermarkProcessingMark {
>> 80:   HandleMark _hm;
> 
> Since you're removing the ResetNoHandleMark, you should revisit
> the comment block for the class. The entire sentence to revisit is:
> // ... It is mostly
> // due to problems that we might want to eventually clean up inside of the
> // frame processing code, such as creating random handles even though there
> // is no safepoint to protect against, and fiddling around with exceptions.
> 
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 493:
> 
>> 491:     ThreadInVMfromNative __tiv(thread);                              \
>> 492:     debug_only(VMNativeEntryWrapper __vew;)                          \
>> 493:     debug_only(ResetNoHandleMark __rnhm;)                            \
> 
> Your fix for JDK-8259374 adds a ResetNoHandleMark to
> ThreadInVMfromNative so new L493 isn't needed.
> 
> -------------
> 
> Changes requested by dcubed (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/1990
>