RFR: 8277444: Data race between JvmtiClassFileReconstituter::copy_bytecodes and class linking [v5]

[David Holmes]

On Tue, 2 Sep 2025 21:45:03 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:

>> There is a race between `JvmtiClassFileReconstituter::copy_bytecodes` and `InstanceKlass::link_class_impl`.  `InstanceKlass::link_class_impl` can be rewriting bytecodes. `JvmtiClassFileReconstituter::copy_bytecodes` will not restore them to the original ones because the flag `rewritten` is `false`. This will result in invalid bytecode.
>> 
>> This PR adds linking a class before the `copy_bytecodes` method is called.
>> The PR also adds a regression test.
>> 
>> Tested fastdebug and release builds: Linux x86_64 and arm64
>> - The reproducer from JDK-8277444 passed.
>> - The regression test passed.
>> - Tier1 - tier3 passed.
>
> Evgeny Astigeevich has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Switch to macros HAS_PENDING_EXCEPTION, CLEAR_PENDING_EXCEPTION

Generally looks good. There are a few minor nits/typos.

I'm not sure about the test, in particular the attempt to calculate `MIN_LINK_TIME_MS`. It is very hard to see/know that you will actually induce the desired race. But as the test doesn't actually have any explicit failure conditions, it at least won't generate false reports.

Thanks

src/hotspot/share/prims/jvmtiEnv.cpp line 454:

> 452:     if (ik->get_cached_class_file_bytes() == nullptr) {
> 453:       // Link the class to avoid races with the rewriter. This will call the verifier also
> 454:       // on the class. Linking is done already below in VM_RedefineClasses below, but we need

Suggestion:

      // on the class. Linking is also done in VM_RedefineClasses below, but we need

There are two "below"s

test/jdk/java/lang/instrument/RetransformBigClassTest.java line 44:

> 42:  * in another thread. The test uses Class.forName("BigClass", false, classLoader)
> 43:  * which does not link the class. When the class is used, the linking process starts.
> 44:  * In another thread retransforming of the class is happening,

Suggestion:

 * In another thread retransforming of the class is happening.

test/jdk/java/lang/instrument/RetransformBigClassTest.java line 45:

> 43:  * which does not link the class. When the class is used, the linking process starts.
> 44:  * In another thread retransforming of the class is happening,
> 45:  * We generate a class with big methods. A number of methods and thier size are

Suggestion:

 * We generate a class with big methods. A number of methods and their size are

test/jdk/java/lang/instrument/RetransformBigClassTest.java line 46:

> 44:  * In another thread retransforming of the class is happening,
> 45:  * We generate a class with big methods. A number of methods and thier size are
> 46:  * chosen to make the linking and retransforming processes running concurrently.

Suggestion:

 * chosen to make the linking and retransforming processes run concurrently.

test/jdk/java/lang/instrument/RetransformBigClassTest.java line 54:

> 52:     private static final Object LOCK = new Object();
> 53:     private static final int COUNTER_INC_COUNT            = 2000; // A number of 'c+=1;' statements in methods of a class.
> 54:     private static final int MIN_LINK_TIME_MS             = 60;   // This time is chosen to be big enough the linking and retransforming processes are running in parallel.

Suggestion:

    private static final int MIN_LINK_TIME_MS             = 60;   // Large enough so linking and retransforming run in parallel.

test/jdk/java/lang/instrument/RetransformBigClassTest.java line 108:

> 106:     }
> 107: 
> 108:     // We calculate a number of methods the linking time to exceed MIN_LINK_TIME_MS.

I can't quite parse this sentence.

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

PR Review: https://git.openjdk.org/jdk/pull/26863#pullrequestreview-3182975103
PR Review Comment: https://git.openjdk.org/jdk/pull/26863#discussion_r2320540581
PR Review Comment: https://git.openjdk.org/jdk/pull/26863#discussion_r2320542950
PR Review Comment: https://git.openjdk.org/jdk/pull/26863#discussion_r2320543345
PR Review Comment: https://git.openjdk.org/jdk/pull/26863#discussion_r2320543595
PR Review Comment: https://git.openjdk.org/jdk/pull/26863#discussion_r2320544630
PR Review Comment: https://git.openjdk.org/jdk/pull/26863#discussion_r2320546835