On Tue, 16 Sep 2025 21:45:37 GMT, Leonid Mesnik <[email protected]> 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
