RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS
Jini George
jini.george at oracle.com
Wed Apr 25 03:26:20 UTC 2018
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