[jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v2]

[David Holmes]

On Tue, 13 Jul 2021 15:16:26 GMT, Jorn Vernee <jvernee at 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