On Tue, 10 Mar 2026 12:53:48 GMT, Zhengyu Gu <[email protected]> wrote:
>> Please review this change that fixes crashes by jmm_GetThreadInfo() call.
>>
>> There are several issues:
>> - ThreadIdTable::lazy_initialize() has typical double-checked locking
>> pattern without proper memory barrier, that may result in
>> uninitialized/partial initialized table to be observed by other threads.
>> Added release/acquire barrier to address this issue.
>> - Query ThreadIdTable can race add/remove thread operations. In short, the
>> thread returned from the query may be freed. Fortunately,
>> jmm_GetThreadInfo() acquires stable thread list before query, so we only
>> need to make sure that returned thread is in the list (checking
>> thread->is_exiting() does not help due to the race)
>> - I moved thread Id insertion code from ThreadSMR to Threads, to be
>> symmetric to thread Id removal code.
>>
>> Tests:
>> - [x] Tier1 on Linux and MacOSX (fastdebug)
>> (`tools/javac/annotations/typeAnnotations/IncorrectCastOffsetTest.java`
>> failure seems unrelated, it also fails in master)
>
> Zhengyu Gu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> @dholmes-ora's comment
Changes requested by dholmes (Reviewer).
src/hotspot/share/runtime/threadSMR.cpp line 730:
> 728: }
> 729: } else if (includes(thread) && !thread->is_exiting()) {
> 730: // The thread is protected by this list and has yet exited
Suggestion:
// The thread is protected by this list and has not yet exited
src/hotspot/share/services/threadIdTable.cpp line 46:
> 44: static volatile size_t _items_count = 0;
> 45:
> 46: Atomic<bool> ThreadIdTable::_is_initialized(false);
Suggestion:
Atomic<bool> ThreadIdTable::_is_initialized{false};
I don't know why but the brace initializer seems to be preferred here - see
e.g. https://github.com/openjdk/jdk/pull/30130/files
src/hotspot/share/services/threadIdTable.cpp line 88:
> 86: // to be concurrently populated during initialization.
> 87: MutexLocker ml(ThreadIdTableCreate_lock);
> 88: if (_is_initialized.load_relaxed()) {
It looks odd to me to mix raw accesses and accessor functions - plus we prefer
accessors/settors to explicitly indicate when they have acquire/release
semantics, I would suggest not having the `is_initialized()` method - and only
this Line 83 usage actually requires the acquire semantics the others can be
load_relaxed.
-------------
PR Review: https://git.openjdk.org/jdk/pull/30105#pullrequestreview-3926435052
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2915441549
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2915459617
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2915478026
