RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS

David Holmes david.holmes at oracle.com
Tue Apr 24 09:59:33 UTC 2018


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