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

[Chris Plummer]

On Mon, 25 Jan 2021 09:34:00 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> I noticed that the SA's dumpreplaydata command fails with:
>> 
>> java.lang.AssertionError: CLHSDB wasn't run successfully: Opening core file, please wait...
>> hsdb> Exception in thread "main" java.lang.InternalError: ciMetadata does not appear to be polymorphic
>> 
>> with a simple test program. This happens because the SA can't find the
>> vtable symbol for ciMetadata (build produced by gcc 9.2.1). AFAIU,
>> there's nothing in our build system that hides that symbol. I had to
>> move one method's definition from the header file to the cpp file for
>> the symbol to be visible again.
>> 
>> We have a test that checks dumpreplaydata but it doesn't catch that
>> problem. The test produces a replay file from a core file with the SA
>> by running a simple test with -Xcomp and CICrash=1. So the replay data
>> has very little or no profile data (which is what causes the problem
>> above). I propose running a slightly more complicated test method and
>> crashing after the method has had time to run for long enough to
>> collect profile data.
>> 
>> The other shortcoming of the test is that it doesn't look at the
>> content of the replay file. It only warns if they differ. The replay
>> file produced by the VM and the one produced by the SA should be
>> identical (except for comment lines). So I propose we check that.
>> 
>> Finally, I can't run that test on my system because core files are
>> handled by systemd (I'm running some recent version of fedora). I
>> suppose, the system can be configured differently but having the test
>> work out the box is nice. I extended the test case to handle that.
>> 
>> With the improved test, there are a few differences between the VM and
>> SA replay files caused by VM changes that were not mirrored in the
>> SA. I fixed those.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   use CoreUtils

test/lib/jdk/test/lib/util/CoreUtils.java line 193:

> 191:     public static String getCoreFileLocation(String crashOutputString) throws IOException {
> 192:         return getCoreFileLocation(crashOutputString, -1);
> 193:     }

In order to be consistent, it seems that all users of `getCoreFileLocation()` should now be passing in the pid and we should get rid of this version that doesn't require it. That means they also all need to be converted to use `ProcessTools.executeProcessPreservePID()`.

test/lib/jdk/test/lib/util/CoreUtils.java line 176:

> 174:                                 Asserts.assertGT(coreFile.length(), 0L, "Unexpected core size");
> 175:                                 System.out.println("coredumpctl succeeded");
> 176:                                 return core;

At the start of the outermost if/else block, there is a comment that says "See if we can figure out the likely reason the core file was not found". However, now we are doing further attempts to find the core file. At the very least the comment should be updated. Maybe add something like "Recover from failure if possible."

test/hotspot/jtreg/compiler/ciReplay/CiReplayBase.java line 182:

> 180:         if (needCoreDump) {
> 181:             try {
> 182:                 String coreFileLocation = CoreUtils.getCoreFileLocation(crashOutputString);

Don't you need to pass in `pid` here?

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

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