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

[Chris Plummer]

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
>