RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS
Yasumasa Suenaga
yasuenag at gmail.com
Wed Apr 25 12:44:43 UTC 2018
Hi Jini,
>>>>>> 2018-04-18 15:05 GMT+09:00 Jini George <jini.george at 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.george at 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.
>>>>>>>>>
>>>>>>>>>
>>>>>>>
More information about the serviceability-dev
mailing list