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

[Chris Plummer]

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