RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS
Jini George
jini.george at oracle.com
Thu Apr 26 04:21:30 UTC 2018
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.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 hotspot-runtime-dev
mailing list