Hi Jini, I have no further comment.
Yasumasa 2018-04-26 13:21 GMT+09:00 Jini George <jini.geo...@oracle.com>: > Thank you, Yasumasa. I hope to implement the consolidation with > TestSAServer.java (and have the SA core file debug testing template done) as > a part of a separate enhancement: > https://bugs.openjdk.java.net/browse/JDK-8202297 > > Let me know if this is not OK with you. > > Thanks, > Jini. > > > On 4/25/2018 6:14 PM, Yasumasa Suenaga wrote: >> >> Hi Jini, >> >> >>>>>>>> 2018-04-18 15:05 GMT+09:00 Jini George <jini.geo...@oracle.com>: >> >> >> : >> >>>>>>>>> I plan to file an enhancement request to address this issue (wrt >>>>>>>>> systemd-coredump) separately since this would apply to other >>>>>>>>> coredump >>>>>>>>> generating test cases also like: >>>>>>>>> test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java. >> >> >> >> I guessed the tests for coredumps will be handled in another issue (with >> TestSAServer.java). >> Is it okay to implement coredump test in this changeset? >> >> IMHO, it looks to implement as new test basis (e.g. LingeredAppForCoredump >> - ulimit check, set, etc...). >> >> >> Thanks, >> >> Yasumasa >> >> >> >> On 2018/04/25 12:26, Jini George wrote: >>> >>> Thank you very much, David for looking into this. I have incorporated all >>> the comments and the revised webrev is at: >>> >>> http://cr.openjdk.java.net/~jgeorge/8174994/webrev.02/index.html >>> >>> Thanks, >>> Jini. >>> >>> On 4/24/2018 3:29 PM, David Holmes wrote: >>>> >>>> Hi Jini, >>>> >>>> Not a full review as I'm not familiar enough with this code. >>>> >>>> My main comment, again, relates to >>>> test/hotspot/jtreg/serviceability/sa/ClhsdbCDSCore.java and that it should >>>> not fail (throw Error) if there is no core file generated etc. In that case >>>> the test should be skipped with a clear message (as elsewhere). Otherwise >>>> this test will fail locally for me every time I run the serviceability >>>> tests! >>>> >>>> I also have a few style issues. >>>> >>>> Don't compare boolean functions with true or false i.e. >>>> >>>> if (isX() == true) -> if (isX()) >>>> if (isX() == false) -> if (!isX()) >>>> >>>> this occurs in most of the Java files. It is especially noticeable when >>>> you mix styles ie: >>>> >>>> + if (VM.getVM().isSharingEnabled()) { <= implicit check of true >>>> + // Check if the value falls in the _md_region >>>> + FileMapInfo cdsFileMapInfo = VM.getVM().getFileMapInfo(); >>>> + if (cdsFileMapInfo.inCopiedVtableSpace(loc1) == true) { <= >>>> explicit check >>>> + return cdsFileMapInfo.getTypeForVptrAddress(loc1); >>>> + } >>>> + } >>>> >>>> --- >>>> >>>> >>>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/FileMapInfo.java >>>> >>>> 139 vTableTypeMap.put >>>> 140 (copiedVtableAddress.addOffsetTo(VM.getVM().getAddressSize()), >>>> metadataTypeArray[i]); >>>> 141 // The '+ 1' below is to skip the entry containing the >>>> size of this metadata's vtable. >>>> 142 copiedVtableAddress = >>>> 143 copiedVtableAddress.addOffsetTo((metadataVTableSize + 1) >>>> * VM.getVM().getAddressSize()); >>>> >>>> If you store VM.getVM().getAddressSize() in a local you only need call >>>> it once, and the other lines of code will be shorter. >>>> >>>> On line 139/140 keep the opening parenthesis with the method name ie: >>>> >>>> vTableTypeMap.put( >>>> >>>> but with shorter lines you should be able to reformat that more cleanly >>>> anyway. >>>> >>>> >>>> 146 } // FileMapHeader >>>> 147 } // FileMapInfo >>>> >>>> We generally don't comment the end of blocks. >>>> >>>> --- >>>> >>>> test/hotspot/jtreg/serviceability/sa/ClhsdbCDSCore.java >>>> >>>> 96 } catch (Throwable t) { >>>> 97 throw new Error("Can't execute the java cds >>>> process."); >>>> 98 } >>>> >>>> Set 't' as the cause of the new Error so we can see why it failed. >>>> >>>> Thanks, >>>> David >>>> >>>> On 24/04/2018 7:03 PM, Jini George wrote: >>>>> >>>>> Hello! >>>>> >>>>> The webrev including the check for the "|" at the beginning of the >>>>> core_pattern file is at: >>>>> >>>>> http://cr.openjdk.java.net/~jgeorge/8174994/webrev.01/ >>>>> >>>>> This webrev also includes a fix for a latent bug on MacOSX, where >>>>> corefile debugging was failing due to SA trying to read in the incorrect >>>>> mangled symbol name for "Arguments::SharedArchivePath". Clang seems to >>>>> have >>>>> prefixed an extra '_' to change the mangled name from >>>>> '_ZN9Arguments17SharedArchivePathE' to >>>>> '__ZN9Arguments17SharedArchivePathE' >>>>> for MachO files. This fix for this is in >>>>> src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c. >>>>> >>>>> The difference between the earlier patch and this one can be seen at: >>>>> >>>>> http://cr.openjdk.java.net/~jgeorge/8174994/differential.patch >>>>> >>>>> Thank you, >>>>> Jini. >>>>> >>>>> >>>>> On 4/18/2018 10:37 PM, Jini George wrote: >>>>>> >>>>>> I agree with the need of testing as much as we can. I could do >>>>>> something on the lines of how other debuggers like LLDB test: if we can't >>>>>> find the core file location, check for "|" at the beginning of a line in >>>>>> the >>>>>> /proc/sys/kernel/core_pattern file -- and fail with a message stating >>>>>> that >>>>>> the system is using a crash reporting tool. >>>>>> >>>>>> Thank you, >>>>>> Jini. >>>>>> >>>>>> On 4/18/2018 12:40 PM, David Holmes wrote: >>>>>>> >>>>>>> My 2c ... >>>>>>> >>>>>>> We have to have tests that can test core file attaching capability - >>>>>>> else we don't know it works. So we have to try and generate a core file. >>>>>>> >>>>>>> But, we have to expect that in many cases no core file will be >>>>>>> generated even if the hs-err file claims it was. For example my primary >>>>>>> local testing system never generates core files even though it claims >>>>>>> to: >>>>>>> >>>>>>> # Core dump will be written. Default location: Core dumps may be >>>>>>> processed with "/usr/share/apport/apport %p %s %c" (or dumping to / >>>>>>> >>>>>>> export/users/dh198349/valhalla/repos/valhalla-dev/open/test/jdk/core.29848) >>>>>>> >>>>>>> apport isn't even installed, even though core_pattern lists it. >>>>>>> >>>>>>> Cheers, >>>>>>> David >>>>>>> >>>>>>> On 18/04/2018 4:36 PM, Yasumasa Suenaga wrote: >>>>>>>> >>>>>>>> 2018-04-18 15:05 GMT+09:00 Jini George <jini.geo...@oracle.com>: >>>>>>>>> >>>>>>>>> Thank you very much, Yasumasa, for pointing this out. You are right >>>>>>>>> -- this >>>>>>>>> would fail in the Linux systems if systemd-coredump is enabled. >>>>>>>>> >>>>>>>>> I plan to file an enhancement request to address this issue (wrt >>>>>>>>> systemd-coredump) separately since this would apply to other >>>>>>>>> coredump >>>>>>>>> generating test cases also like: >>>>>>>>> test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java. >>>>>>>> >>>>>>>> >>>>>>>> I agree with you, but... >>>>>>>> >>>>>>>>> From what i can gather, i think we might be able to at least >>>>>>>>> partially >>>>>>>>> address this by using >>>>>>>>> >>>>>>>>> coredumptl -o <desired_core_path> dump <pid of the crashed process> >>>>>>>>> >>>>>>>>> in the test cases, provided the kernel.core_pattern variable is not >>>>>>>>> set to >>>>>>>>> "|/bin/false". >>>>>>>>> >>>>>>>>> Let me know if you are not OK with this. >>>>>>>> >>>>>>>> >>>>>>>> IMHO it is not good. >>>>>>>> Some Linux distros use other coredump collector. For example, RHEL 6 >>>>>>>> uses ABRT, Ubuntu uses Apport, Fedora uses systemd-coredump. >>>>>>>> Hence I think we should disable all tests which requires core images >>>>>>>> for Linux like a Windows platform. >>>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Yasumasa >>>>>>>> >>>>>>>> >>>>>>>>> Thank you, >>>>>>>>> Jini. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 4/14/2018 7:39 PM, Yasumasa Suenaga wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Hi Jini, >>>>>>>>>> >>>>>>>>>> ClhsdbCDSCore.java: >>>>>>>>>> Can this test work on modern Linux? >>>>>>>>>> AFAIK modern Linux contains systemd-coredump to gather core >>>>>>>>>> images. So >>>>>>>>>> I concern ClhsdbCDSCore.java fails in the future. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> >>>>>>>>>> Yasumasa >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 2018/04/12 13:21, Jini George wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Ping: Gentle reminder ! >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Jini. >>>>>>>>>>> >>>>>>>>>>> On 4/6/2018 9:51 PM, Jini George wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Hello! >>>>>>>>>>>> >>>>>>>>>>>> Requesting reviews for: >>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8174994 >>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~jgeorge/8174994/webrev.00/ >>>>>>>>>>>> >>>>>>>>>>>> While trying to identify the type given an address, a >>>>>>>>>>>> WrongTypeException >>>>>>>>>>>> was getting thrown with various clhsdb commands (like printmdo, >>>>>>>>>>>> jstack, >>>>>>>>>>>> etc). This was since SA tries to map an address to a hotspot C++ >>>>>>>>>>>> type by >>>>>>>>>>>> comparing the vtable address to the vtable address values of >>>>>>>>>>>> known >>>>>>>>>>>> types. With CDS, since the vtables are copied over for the >>>>>>>>>>>> Metadata >>>>>>>>>>>> classes, the vtable addresses themselves don't match (though, of >>>>>>>>>>>> course, >>>>>>>>>>>> the contents will), and SA errors out. >>>>>>>>>>>> >>>>>>>>>>>> The fix has been implemented by making changes to read in the md >>>>>>>>>>>> region >>>>>>>>>>>> (consisting of the c++ vtables) of the CDS archive in SA, and >>>>>>>>>>>> mapping >>>>>>>>>>>> the vtable addresses to the corresponding metadata type >>>>>>>>>>>> (ConstantPool, >>>>>>>>>>>> InstanceKlass, InstanceClassLoaderKlass, InstanceMirrorKlass, >>>>>>>>>>>> InstanceRefKlass, Method, ObjArrayKlass, TypeArrayKlass). >>>>>>>>>>>> >>>>>>>>>>>> For corefiles, an additional modification has been done to have >>>>>>>>>>>> the >>>>>>>>>>>> replicated FileMapHeader structure (from >>>>>>>>>>>> src/hotspot/share/memory/filemap.hpp, which is replicated in SA >>>>>>>>>>>> in >>>>>>>>>>>> ps_core.c), to be in sync with the corresponding definition in >>>>>>>>>>>> src/hotspot/share/memory/filemap.hpp. >>>>>>>>>>>> >>>>>>>>>>>> Test cases to test both live and corefile debugging are being >>>>>>>>>>>> added with >>>>>>>>>>>> this. These and other SA tests pass on Mach5. >>>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> Jini. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> >
