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

[Denghui Dong]

On Tue, 10 Sep 2024 22:10:16 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> Liang Mao has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Exclude libagent8339725.cpp compiling for windows
>
> 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?

removed

> 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.

updated.

> 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.

updated.

> 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.

updated.

> 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.

Changed the way to get the byte codes of the target class.

> 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?

Moved this static field into `ClassPrepareCallback`, and only one thread can modify it.

> 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.

updated.

> 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.

updated.

> 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.

Added result check for all jvmti calls except `Deallocate`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1753650744
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1753652777
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1753650356
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1753658539
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1753661825
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1753657463
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1753653794
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1753654138
PR Review Comment: https://git.openjdk.org/jdk/pull/20907#discussion_r1753665549