RFR (Preliminary): 8248194: Need better support for running SA tests on core files

Wed Jul 1 17:48:32 UTC 2020

Good idea, I think. 

So we will have core utils which allow to ensure that core is dumped and help to find them.

Leonid

> On Jun 29, 2020, at 5:10 PM, Chris Plummer <chris.plummer at oracle.com> wrote:
> 
> Hi Leonid,
> 
> I'm starting to think that this should all go in a new CoreUtils.java file. I experimented with moving getCoreFileLocation() to OutputAnalyzer. It worked, but one adjustment I had to make is also moving SATestUtils.unzipCores() there also and make it private (no one else calls it). But that just got me thinking that maybe CoreUtils.java would be a better solution. I think I would put the addCoreUlimitCommand() API discussed below there also. What do you think?
> 
> thanks,
> 
> Chris
> 
> On 6/28/20 5:29 PM, Chris Plummer wrote:
>> Hi Leonid,
>> 
>> I think getCoreFileLocation() can simply move to OutputAnalyzer. No need for it to be in SAUtils and be passed the String argument that comes from OutputAnalyzer.getOutput().
>> 
>> For the ulimit support, how about if in ProcessTools I add:
>> 
>>     public static ProcessBuilder addCoreUlimitCommand(ProcessBuilder pb);
>> 
>> All the ulimit logic would move there from SATestUtils. It's straight forward to use this API in LingeredApp and TestJmapCore. For ClhsdbCDSCore I'll need to rework the getTestJvmCommandlineWithPrefix() code a bit, since it creates a pb, but doesn't save it. It only uses it to get the cmd String.
>> 
>> Also, there's one new finding since I sent out the review. I found the following in CiReplayBase.java:
>> 
>>     // lets search few possible locations using process output and return existing location
>>     private String getCoreFileLocation(String crashOutputString) {
>> 
>> This is identical to the code I pulled from ClhsdbCDSCore and is now in SATestUtils.parseCoreFileLocationFromOutput(). Although this is in the compiler directory, it is in fact an SA test that uses clhsdb, although directly via the CLHSDB class rather than through "jhsdb clhsdb".
>> 
>> This also explains why ClhsdbCDSCore had some logic to move and rename the core file to "cds_core_file". I removed this logic because it seemed unnecessary, but for CiReplayBase.java it needs to be in a known location so SABase.java can find it. It's still fine for ClhsdbCDSCore to not do the rename, and renaming is independent of any code that locates the core file.
>> 
>> I'm not going to update CiReplayBase.java as part of these changes because the two tests that use it both have issues. TestSAServer is problem listed, and when I removed it from the problem list it failed with every run on every platform. There's also TestSAClient, but it relies on client VM, which we don't support anymore. So with neither of these tests running, I'd rather not introduce changes I can't really test.
>> 
>> However, there was something good that came out of the CiReplayBase.java discovery. I had previously noted that ClhsdbCDSCore is excluded from running on windows. When I removed the @requires for this, it failed for a reason I didn't quite understand. The complaint was about the path to java.exe when running the process that was suppose to crash, although the path looked fine. However, I found that TestSAServer ran fine on Windows, even though it was basically the process launching code for causing the crash. I looked closer and found one difference. In getTestJvmCommandlineWithPrefix(), which both tests have, the CiReplayBase version had some extra code for Windows:
>> 
>>             return new String[]{"sh", "-c", prefix
>>                 + (Platform.isWindows() ? cmd.replace('\\', '/').replace(";", "\\;").replace("|", "\\|") : cmd)};
>> 
>> So on Windows it's doing a path conversion. Once I started doing the same with ClhsdbCDSCore, it started to run fine on Windwos also.
>> 
>> thanks,
>> 
>> Chris
>> 
>> On 6/26/20 8:42 PM, Chris Plummer wrote:
>>> Hi Leonid,
>>> 
>>> On 6/26/20 7:51 PM, Leonid Mesnik wrote:
>>>> Hi
>>>> 
>>>> The idea basically looks good. I think it just make a sense to polish it a little bit to hide "sh" usage from test and get core from OutputAnalyzer.
>>> Ok, I'll look into both of those.
>>>>   Also, there is a 'CrashApp' in ClhsdbCDSCore.java. Makes it sense to unify it with LingeredApp crasher? Currently, it uses Unsafe to crash application.
>>> Yes, I purposely didn't not make that change. My main goal with the LingeredApp changes is to make it easier to make existing LingeredApp SA tests run on both a live process and on a core, and my main goal with ClhsdbCDSCore and TestJmapCore was to move the core finding code and ulimit code to a common location that could be reused by other tests.
>>> 
>>> Keep in mind that ClhsdbLauncher and LingeredApp are independent of each other. You can have a LingeredApp tests that use or don't use ClhsdbLauncher, and you can have a non-LingeredApp tests that use or don't use ClhsdbLauncher. So I didn't want to go down the path of changing ClhsdbCDSCore (a non LingeredApp test) to use LingeredApp. Likewise I did not change TestJmapCore to use LingeredApp or ClhsdbLauncher. Possibly there is good reason to convert some of the tests to start using LingeredApp and/or ClhsdbLauncher, but that should be done under a separate RFE.
>>> 
>>>> 
>>>> Also, crashes are used in other tests, I see some implementations in
>>>> open/test/hotspot/jtreg/vmTestbase/vm/share/vmcrasher
>>> I don't see vmcrasher being used by any tests. In any case, my first attempt went down the Unsafe path to produce a crash. The issue is that it forces every user of LingeredApp to include the @module for Unsafe. I also tried using a WhiteBox API. That was worse, also requiring every user of LingeredApp to include an @module, plus the tests that actually want to cause a crash need to @build WhiteBox.java and then do the classfile install. It also required additional module related hacks in LingeredApp. The issue with my current solution is how to get libLingeredApp.c to compile has not been settled on. I'm still waiting for an answer from the build team.
>>>> 
>>>> So it would be nice to have some common way to crash hotspot.
>>> I can see possibly moving the crashing code out of LingeredApp and into a native lib that non-LingeredApp tests can use, although that really is just a very small part of the changes to LingeredApp. For the most part the changes would look the same except you would call a different API to cause the crash.
>>>> 
>>>> Leonid
>>>> 
>>> Thanks for having a look.
>>> 
>>> Chris
>>>>> On Jun 25, 2020, at 2:41 PM, Chris Plummer <chris.plummer at oracle.com> wrote:
>>>>> 
>>>>> Hello,
>>>>> 
>>>>> Please help with a preliminary review of changes to add better support for writing SA tests that work on core files:
>>>>> 
>>>>> https://bugs.openjdk.java.net/browse/JDK-8248194
>>>>> http://cr.openjdk.java.net/~cjplummer/8248194/webrev.00/index.html
>>>>> 
>>>>> As pointed out, this is a preliminary review. I suspect there will be some feedback for changes/improvements. Also, I still need to work out a final solution for how to get LingeredApp to produce a crash. What I currently have works but is somewhat of a hack w.r.t. the makefile change, so you can ignore the makefiile change for now. I'm working on a more proper solution with the build team.
>>>>> 
>>>>> As outlined in the CR, these are the 3 main goals of this CR:
>>>>> 
>>>>> 1. SATestUtils should include support for finding the core file. This includes parsing the output of the crashed process to locate where the core file was saved, and returning this location to the user.
>>>>> 
>>>>> 2. SATestUtils should include support for adding the "ulimit -c unlimited" prefix to the command that will produce the core file, allowing the overriding of any lower limit so we can be sure the core file will be produced.
>>>>> 
>>>>> 3. LingeredApp should include support for producing a core file.
>>>>> 
>>>>> As proof of concept for these 3 changes in test library support, I'm updating the following 3 tests:
>>>>> 
>>>>> ClhsdbCDSCore.java: Use the SATestUtils support listed above. This test does not use LingeredApp, so those improvements don't apply.
>>>>> 
>>>>> TestJmapCore.java: Use the SATestUtils support listed above. This test does not use LingeredApp, so those improvements don't apply.
>>>>> 
>>>>> ClhsdbFindPC.java: Use all the above features, including having LingeredApp produce a core file. This is the only test modified to start testing on core files that didn't previously do so. It still also tests on a live process.
>>>>> 
>>>>> In the future more Clhsdb tests will be converted to work on core files in a manner similar to ClhsdbFindPC.
>>>>> 
>>>>> The new SATestUtils code is borrowed from (more like ripped out of) ClhsdbCDSCore.java and TestJmapCore.java. They both had a lot of code dedicated to finding the core file and also applying "ulimit -c unlimitted" if necessary, but didn't do so in quite the same way. Now both these tests share code in SATestUtils.java. One thing I did drop is TestJmapCore.java use of ":KILLED_PID" in the output to help find the core file. It's no longer necessary based on the smarter core locating code I pulled from ClhsdbCDSCore.java.
>>>>> 
>>>>> thanks,
>>>>> 
>>>>> Chris
>>> 
>> 
>> 
> 
> 