On Tue, 2 Sep 2025 21:45:03 GMT, Evgeny Astigeevich <eastigeev...@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
