RFR: 8367725: Incorrect reading of oop in SuspendResumeManager::suspend while thread is blocked [v8]

[David Holmes]

On Tue, 16 Sep 2025 21:45:37 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> The
>> `SuspendResumeManager::suspend(bool register_vthread_SR)`
>> has an issue while suspend current virtual thread. The suspend tries to access vthread oop field to read vthread id after thread is blocked. 
>> 
>> Seems, that this case is not used by our debugger and was not covered by tests. I found it using jtreg test thread virtual factory plugin. I updated existing test to reproduce this problem. The easiest way is to suspend current virtual thread using plain SuspendThread. 
>> 
>> The fix added some "asymmetry"  in suspend/resume mechanism which is required because self-suspend doesn't have resume counterpart.
>
> Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   test fixed

Good find! Always a worry when lack of test coverage is exposed this way.

A couple of suggestions.

Thanks

src/hotspot/share/runtime/suspendResumeManager.cpp line 100:

> 98:     // If target is the current thread we can bypass the handshake machinery
> 99:     // and just suspend directly.
> 100:     // All required data should be loaded before state is set to _thread_blocked.

The comment is a bit vague. Suggestion

// The vthread() oop must only be accessed before state is set to _thread_blocked.

src/hotspot/share/runtime/suspendResumeManager.cpp line 107:

> 105:     do_owner_suspend();
> 106:     return true;
> 107:   }

I prefer to see the `else` remain.

src/hotspot/share/runtime/suspendResumeManager.hpp line 61:

> 59: 
> 60:   void set_suspended(bool to, bool register_vthread_SR);
> 61:   void set_suspended_with_id(int64_t id, bool register_vthread_SR);

Some descriptive comments would be useful now due to the difference in meaning for "suspended". Personally I think we should have `set_suspended` and `set_resumed` rather than passing true/false for the suspend state.

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

PR Review: https://git.openjdk.org/jdk/pull/27317#pullrequestreview-3232561397
PR Review Comment: https://git.openjdk.org/jdk/pull/27317#discussion_r2354226780
PR Review Comment: https://git.openjdk.org/jdk/pull/27317#discussion_r2354227402
PR Review Comment: https://git.openjdk.org/jdk/pull/27317#discussion_r2354243858