On Sun, 15 Mar 2026 00:46:19 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:
>
> Fix test
Can you add a comment explaining the basic test operation please. I can't quite
figure out the roles of the different threads (and I see you just removed one
of them). Also it seems the `ids` array is potentially being used concurrently
from different threads with no synchronization and no use of volatiles (not
that you can have a volatile array). Thanks.
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 49:
> 47: private static final int NUM_THREADS = 2;
> 48: static long [] ids = new long[NUM_THREADS];
> 49: static ThreadInfo [] infos = new ThreadInfo[NUM_THREADS];
Suggestion:
static long[] ids = new long[NUM_THREADS];
static ThreadInfo[] infos = new ThreadInfo[NUM_THREADS];
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 81:
> 79: private static Thread newThread(int i) {
> 80: return new MyThread(i);
> 81: }
This function is not needed.
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 83:
> 81: }
> 82:
> 83: private static void startThreads(long [] ids, int count) {
Suggestion:
private static void startThreads(long[] ids, int count) {
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 95:
> 93:
> 94: // Show ThreadInfo from array, return how many are non-null.
> 95: private static int showInfo(long [] ids, ThreadInfo [] info) {
Suggestion:
private static int showInfo(long[] ids, ThreadInfo[] info) {
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 106:
> 104: i++;
> 105: }
> 106: }
Indentation is wrong.
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 109:
> 107: return count;
> 108: }
> 109: private static int replaceThreads(long [] ids, ThreadInfo [] info,
> int maxToReplace) {
Suggestion:
private static int replaceThreads(long[] ids, ThreadInfo[] info, int
maxToReplace) {
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 137:
> 135:
> 136: static class MyReplacerThread extends Thread {
> 137: long [] ids;
Suggestion:
long[] ids;
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 139:
> 137: long [] ids;
> 138:
> 139: public MyReplacerThread(long []ids) {
Suggestion:
public MyReplacerThread(long[] ids) {
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 185:
> 183:
> 184: static class MyGetThreadInfoThread extends Thread {
> 185: long [] ids;
Suggestion:
long[] ids;
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 187:
> 185: long [] ids;
> 186:
> 187: public MyGetThreadInfoThread(long [] ids) {
Suggestion:
public MyGetThreadInfoThread(long[] ids) {
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 215:
> 213: list = new ArrayList<Object>();
> 214: }
> 215: byte [] junk = new byte[1024*104*10];
Suggestion:
byte[] junk = new byte[1024*104*10];
-------------
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/30105#pullrequestreview-3951144049
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937849295
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937859849
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937850483
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937851211
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937852176
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937853087
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937854222
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937854652
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937856165
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937856463
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2937857149
