[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent
From:       Jorn Vernee <jvernee () openjdk ! java ! net>
Date:       2021-06-28 17:44:29
Message-ID: ebTR3Fg3FnrWIJX847RkbgTBN0Ma9rWbdyVR8WR0dhs=.349b6255-e6c7-46d8-b2df-8cd4d4dd13e8 () github ! com
[Download RAW message or body]

On Fri, 25 Jun 2021 17:38:32 GMT, Jorn Vernee <jvernee@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


[prev in list] [next in list] [prev in thread] [next in thread]