Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed [v3]

[Chris Plummer]

On Wed, 24 Apr 2024 02:49:56 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
>
> Serguei Spitsyn has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains four additional 
> commits since the last revision:
> 
>  - “Merge”
>  - review: updated same clarifying comment in several spots
>  - add comments explaining that the vthread() can return outdated oop
>  - 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == 
> target_h()) failed

src/hotspot/share/prims/jvmtiEnvBase.cpp line 2079:

> 2077: void
> 2078: GetSingleStackTraceClosure::do_vthread(Handle target_h) {
> 2079:   // Use jvmti_vthread() instead of vthread() as target could have 
> temporary changed

Suggestion:

  // Use jvmti_vthread() instead of vthread() as target could have temporarily 
changed

src/hotspot/share/prims/jvmtiEnvBase.hpp line 509:

> 507:   void do_vthread(Handle target_h) {
> 508:     assert(_target_jt != nullptr, "sanity check");
> 509:     // Use jvmti_vthread() instead of vthread() as target could have 
> temporary changed

Suggestion:

    // Use jvmti_vthread() instead of vthread() as target could have 
temporarily changed

src/hotspot/share/prims/jvmtiEnvBase.hpp line 531:

> 529:   void do_vthread(Handle target_h) {
> 530:     assert(_target_jt != nullptr, "sanity check");
> 531:     // Use jvmti_vthread() instead of vthread() as target could have 
> temporary changed

Suggestion:

    // Use jvmti_vthread() instead of vthread() as target could have 
temporarily changed

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1577299642
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1577299918
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1577300305