On Fri, 25 Jun 2021 17:38:32 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. > > I've changed these 2 calls to mimic what is done by JavaCallWrapper instead > (with attach and detach included), and removed the old suspend and stack > reguard checks (now handled by the call). > > 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 src/hotspot/cpu/arm/frame_arm.cpp line 320: > 318: return nullptr; > 319: } > 320: FWIW, stubs were missing on some platforms, and this seems to have been fine before, but now started causing compilation errors, so I've added them here. src/hotspot/share/runtime/safepoint.cpp line 931: > 929: // See if return type is an oop. > 930: bool return_oop = nm->method()->is_returning_oop(); > 931: HandleMark hm(self); I was seeing an `assert(_handle_mark_nesting > 1) failed: memory leak: allocating handle outside HandleMark` when the existing code allocates the Handle for `return_value` in the code down below. It seems to be a simple case of a missing handle mark, so I've added it here. (looking at the stack trace in the error log, the caller frame is native code, so I don't think we can expect the caller to have a HandleMark). src/hotspot/share/runtime/thread.cpp line 1974: > 1972: assert(deferred_card_mark().is_empty(), "Should be empty during GC"); > 1973: > 1974: // Traverse the GCHandles I reduced some duplication here while debugging, but this change is not strictly needed (though a nice cleanup, IMO). Let me know, and I could remove this part if preferred. test/jdk/java/foreign/stackwalk/TestAsyncStackWalk.java line 78: > 76: * TestAsyncStackWalk > 77: */ > 78: Suggestion: test/jdk/java/foreign/stackwalk/TestStackWalk.java line 78: > 76: * TestStackWalk > 77: */ > 78: Suggestion: ------------- PR: https://git.openjdk.java.net/jdk17/pull/149
