RFR: 8260296: SA's dumpreplaydata fails [v2]

[Roland Westrelin]

On Fri, 22 Jan 2021 21:28:33 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   use CoreUtils
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ProfileData.java line 54:
> 
>> 52: 
>> 53:   // Low-level accessors for underlying data
>> 54:   long intptrAt(int index) {
> 
> How did this ever work if it was returning an int?

Only the least significant bits are ever looked at so it must have worked but still, seems pretty straightforward to fix.

> test/hotspot/jtreg/compiler/ciReplay/CiReplayBase.java line 179:
> 
>> 177:         }
>> 178:         if (needCoreDump) {
>> 179:             String coreFileLocation = getCoreFileLocation(crashOutputString, pid);
> 
> Please see the comments I just added to JDK-8259501. This test should be using the new core file support in CoreUtils.java. I think this means you don't need any of the ProcessTools.java changes.

Thanks for the pointer. I pushed a new change that refactors the test so it takes advantage of CoreUtils.

> test/hotspot/jtreg/compiler/ciReplay/SABase.java line 144:
> 
>> 142:                     System.out.println("1: " + l1);
>> 143:                     System.out.println("2: " + l2);
>> 144:                     failure = true;
> 
> Don't we want to break out of the loop at this point?

I thought maybe going over the whole files could help diagnose a failure in case, for instance, the only difference would be 2 lines that were swapped.

> test/hotspot/jtreg/compiler/ciReplay/CiReplayBase.java line 314:
> 
>> 312:             // file. It can take a few seconds for the system to
>> 313:             // process the just produced core file so we may need to
>> 314:             // retry a few times.
> 
> If this is really the case, shouldn't all the SA core file tests be running into this problem? Perhaps the following in CoreUtils.java is why they don't:
>         } else if (Platform.isLinux()) {
>             // Check if a crash report tool is installed.
>             File corePatternFile = new File(CORE_PATTERN_FILE_NAME);
>             try (Scanner scanner = new Scanner(corePatternFile)) {
>                 while (scanner.hasNextLine()) {
>                     String line = scanner.nextLine();
>                     line = line.trim();
>                     System.out.println(line);
>                     if (line.startsWith("|")) {
>                         System.out.println(
>                             "\nThis system uses a crash report tool ($cat /proc/sys/kernel/core_pattern).\n" +
>                             "Core files might not be generated. Please reset /proc/sys/kernel/core_pattern\n" +
>                             "to enable core generation. Skipping this test.");
>                         throw new SkippedException("This system uses a crash report tool");
>                     }
>                 }
>             }
>         }

Right. But I still think it's easy enough and useful to support systemd. See the updated change.

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

PR: https://git.openjdk.java.net/jdk/pull/2195