RFR: 8339725: Concurrent GC crashed due to GetMethodDeclaringClass [v5]

[Leonid Mesnik]

On Tue, 10 Sep 2024 10:15:43 GMT, Liang Mao <lmao at openjdk.org> wrote:

>> Hi,
>> 
>> It's a fix for 8339725. I think getting the oop from Klass::java_mirror() should use a ON_PHANTOM_OOP_REF decorator here which could make sure the oop would be kept alive in concurrent marking and return nullptr while in concurrent reference processing and unloading.
>> 
>> test/hotspot/jtreg/runtime and gc are clean.
>> 
>> Thanks,
>> Liang
>
> Liang Mao has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Exclude libagent8339725.cpp compiling for windows

Thanks, for adding test. I was not able to add authors.
See more comments inline. (Mostly stylistic)

test/hotspot/jtreg/runtime/8339725/Test8339725.java line 32:

> 30:  * @requires os.family == "linux"
> 31:  * @library /test/lib
> 32:  * @library /

Why this line is needed?

test/hotspot/jtreg/runtime/8339725/Test8339725.java line 35:

> 33:  * @modules java.base/jdk.internal.misc
> 34:  *          java.management
> 35:  * @run main/othervm/native -agentlib:agent8339725 Test8339725

It should be 
 @run main/driver Test8339725
since you run the test in forked process.

test/hotspot/jtreg/runtime/8339725/Test8339725.java line 53:

> 51:     public static void test(String gcArg) throws Exception {
> 52:         ProcessBuilder pb = ProcessTools.createLimitedTestJavaProcessBuilder(
> 53:             "-agentpath:" + Utils.TEST_NATIVE_PATH + File.separator + System.mapLibraryName("agent8339725"), "-Xmx100m", gcArg, "Test");

Instead of using createLimitedTestJavaProcessBuilder and gcArg, please use 
createTestJavaProcessBuilder and allow system to set arguments. So we can run it in more configurations even if it should work now.
All other test options would be appended to your nativepath and Xmx options.

test/hotspot/jtreg/runtime/8339725/Test8339725.java line 63:

> 61:     public static void main(String[] args) throws Exception {
> 62:         long last = System.nanoTime();
> 63:         for (int i = 0;; i++) {

for (;;) {
looks better if i never used.

test/hotspot/jtreg/runtime/8339725/Test8339725.java line 81:

> 79:     public Class findClass(String name) throws ClassNotFoundException {
> 80:         byte[] b = Base64.getDecoder()
> 81:                 .decode("yv66vgAAADQADgoAAwALBwAMBwANAQAGPGluaXQ+AQADKClWAQAEQ29kZQEAD0xpbmVOdW1iZXJU" +

Please add comment explaining why this string is used as a template.

test/hotspot/jtreg/runtime/8339725/libagent8339725.cpp line 36:

> 34: 
> 35: #define BUFFER_SIZE 100000
> 36: static size_t ring_buffer[BUFFER_SIZE] = {0};

The rung_buffer is shared between threads without any locks, not safe.

test/hotspot/jtreg/runtime/8339725/libagent8339725.cpp line 37:

> 35: #define BUFFER_SIZE 100000
> 36: static size_t ring_buffer[BUFFER_SIZE] = {0};
> 37: static volatile int ring_buffer_idx = 0;

Why volatile is needed here?

test/hotspot/jtreg/runtime/8339725/libagent8339725.cpp line 41:

> 39: 
> 40: void *get_method_details(void *arg)
> 41: {

please move { on the line with declartion, or 'if' 'for' etc in all places.

test/hotspot/jtreg/runtime/8339725/libagent8339725.cpp line 50:

> 48: 
> 49:     // For JVM 17, 21, 22 calling GetMethodDeclaringClass is enough.
> 50:     if ((err = jvmti->GetMethodDeclaringClass(method, &method_class)) == 0)

please use  JVMTI_ERROR_NONE when check error code.

test/hotspot/jtreg/runtime/8339725/libagent8339725.cpp line 53:

> 51:     {
> 52:         // JVM 8 needs this to crash
> 53:         jvmti->GetClassSignature(method_class, &class_name, NULL);

The good practice is to check jvmti status and fails if it is not none.
Check the 'check_jvmti_status' for this from 'jvmti_common.h"
Please check all jvmti functions.

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

Changes requested by lmesnik (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20907#pullrequestreview-2293888922
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752805301
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752811014
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752803305
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752856394
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752860230
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752835741
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752831021
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752814805
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752819319
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1752880408