RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS
David Holmes
david.holmes at oracle.com
Thu Apr 26 04:57:50 UTC 2018
Thanks Jini, I have no further comments as the tests "passed" for me.
David
On 25/04/2018 1:26 PM, 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