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
