On Wed, 17 Apr 2024 00:29:52 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Looks good. Just small suggestion.
Thanks,
Patricio
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2079:
> 2077: void
> 2078: GetSingleStackTraceClosure::do_vthread(Handle target_h) {
> 2079: // use jvmti_vthread() as vthread() can be outdated
The only reason I can see of why just using vthread() doesn't work is because
of the case where we are in a temporary switch to carrier thread. So maybe
change comment to be: "use jvmti_vthread() instead of vthread() as target could
have temporary changed identity to carrier thread (see
VirtualThread.switchToCarrierThread)". Same in the other places.
-------------
Marked as reviewed by pchilanomate (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18806#pullrequestreview-2018092388
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1576768011
