On Tue, 13 Jul 2021 15:16:26 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> This patch rewrites the prologue and epilogue of panama upcalls, in order to
>> fix the test failure from the title.
>>
>> Previously, we did a call to potentially attach the current thread to the
>> VM, and then afterwards did the same suspend and stack reguard checks that
>> we do on the back-edge of a native downcall. Then, on the back edge of the
>> upcall we did another conditional call to detach the thread.
>>
>> The suspend and reguard checks on the front-edge are incorrect, so I've
>> changed the 2 calls to mimic what is done by JavaCallWrapper instead (with
>> attach and detach included), and removed the old suspend and stack reguard
>> checks.
>>
>> FWIW, this removes the JavaFrameAnchor save/restore MacroAssembler code.
>> This is now written in C++. Also, MacroAssembler code was added to
>> save/restore the result of the upcall around the call on the back-edge,
>> which was previously missing. Since the new code allocates a handle block as
>> well, I've added handling for those oops to frame & OptimizedUpcallBlob.
>>
>> Testing: local running of `jdk_foreign` on Windows and Linux (WSL). Tier 1-3
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Assert frame is correct type in frame_data_for_frame
Hi Jorn,
I can't comment on all the details here - especially in the x86_64 upcall
handler code. The mapping to JavaCallWrapper seems reasonable but there are a
few differences that I'm now assuming stem from the fact upcalls start
_thread_in_native while JCW starts from _thread_in_vm?
Some minor comments and queries below (mostly issues with existing code that
you copied).
Thanks,
David
src/hotspot/share/prims/universalUpcallHandler.cpp line 76:
> 74:
> 75: // modelled after JavaCallWrapper::JavaCallWrapper
> 76: Thread*
> ProgrammableUpcallHandler::on_entry(OptimizedEntryBlob::FrameData* context) {
This should return JavaThread not Thread.
src/hotspot/share/prims/universalUpcallHandler.cpp line 77:
> 75: // modelled after JavaCallWrapper::JavaCallWrapper
> 76: Thread*
> ProgrammableUpcallHandler::on_entry(OptimizedEntryBlob::FrameData* context) {
> 77: JavaThread* thread =
> maybe_attach_and_get_thread(&context->should_detach)->as_Java_thread();
Nit: if `maybe_attach_and_get_thread` has to return a JavaThread it should be
typed to return a JavaThread.
src/hotspot/share/prims/universalUpcallHandler.cpp line 86:
> 84: context->new_handles = JNIHandleBlock::allocate_block(thread);
> 85:
> 86: // After this, we are official in Java Code. This needs to be done
> before we change any of the thread local
typo: s/official/officially/
(I see this was copied from Javacalls)
src/hotspot/share/prims/universalUpcallHandler.cpp line 92:
> 90: // Make sure that we handle asynchronous stops and suspends _before_ we
> clear all thread state
> 91: // in OptimizedEntryBlob::FrameData. This way, we can decide if we need
> to do any pd actions
> 92: // to prepare for stop/suspend (flush register windows on sparcs, cache
> sp, or other state).
No sparcs any more (again I see this was copied from Javacalls)
src/hotspot/share/prims/universalUpcallHandler.cpp line 114:
> 112: thread->set_active_handles(context->new_handles); // install new
> handle block and reset Java frame linkage
> 113:
> 114: assert (thread->thread_state() != _thread_in_native, "cannot set
> native pc to NULL");
You transitioned to _thread_in_Java above so how can you possibly have changed
state again ?? (okay again copied from javaCalls ...)
src/hotspot/share/prims/universalUpcallHandler.cpp line 117:
> 115:
> 116: // clear any pending exception in thread (native calls start with no
> exception pending)
> 117: if(clear_pending_exception) {
Nit: space after 'if'
src/hotspot/share/prims/universalUpcallHandler.cpp line 121:
> 119: }
> 120:
> 121: MACOS_AARCH64_ONLY(thread->enable_wx(WXExec));
Not clear why this is needed? JavaCallWrapper doesn't use it.
src/hotspot/share/prims/universalUpcallHandler.cpp line 146:
> 144: // Do this after the transition because this allows us to put an assert
> 145: // the Java->native transition which checks to see that stack is not
> walkable
> 146: // on sparc/ia64 which will catch violations of the reseting of
> last_Java_frame
Again archaic comment copied across :)
src/hotspot/share/runtime/thread.cpp line 1972:
> 1970: (has_last_Java_frame() && java_call_counter() > 0),
> 1971: "unexpected frame info: has_last_frame=%d,
> java_call_counter=%d",
> 1972: has_last_Java_frame(), java_call_counter());
I see this was relocated code, but as has_last_java_frame() returns bool, it
shouldn't be treated as an int without a cast (or better use %s and report true
or false).
-------------
PR: https://git.openjdk.java.net/jdk17/pull/149
