[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with
From: Jini George <jini.george () oracle ! com>
Date: 2018-04-26 6:44:29
Message-ID: ac35ea76-06ff-9ccf-1dfd-3e7cb87f745e () oracle ! com
[Download RAW message or body]
Thank you, Yasumasa.
- Jini.
On 4/26/2018 11:41 AM, Yasumasa Suenaga wrote:
> Hi Jini,
>
> I have no further comment.
>
>
> Yasumasa
>
>
>
> 2018-04-26 13:21 GMT+09:00 Jini George <jini.george@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.george@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@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.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic