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 <[email protected]> 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 <[email protected]> >>>>> 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 >>> >> >> > >
