RFR: 8233285: Demangling C++ symbols in jhsdb jstack --mixed

Chris Plummer chris.plummer at oracle.com
Tue Nov 5 04:47:03 UTC 2019 

On 11/4/19 5:19 PM, Yasumasa Suenaga wrote:
> Hi Chris,
>
> On 2019/11/05 2:28, Chris Plummer wrote:
>> On 11/2/19 5:33 AM, Yasumasa Suenaga wrote:
>>> Hi Chris,
>>>
>>> On 2019/11/02 7:07, Chris Plummer wrote:
>>>> Hi  Yasumasa,
>>>>
>>>> I can't comment on the build changes. I don't know how the build 
>>>> works well enough.
>>>
>>> I ensured it on submit repo 
>>> (mach5-one-ysuenaga-JDK-8233285-1-20191101-0746-6336009).
>>> This change affects for Linux only. So I changed toolchain if SA 
>>> would be built for Linux only.
>>>
>>>
>>>> LinuxDebuggerLocal.cpp should show up as a rename + diffs, not as a 
>>>> new file.
>>>
>>> I uploaded it:
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233285/webrev.00/LinuxDebuggerLocal.patch 
>>>
>>>
>>> If we can use Git, it can show "rename + diffs"...
>>> I tried to use "hg rename" and "hg move", but the result did not 
>>> change.
>> Hi Yasumasa,
>>
>> I just did an "hg mv" and then modified the file, and webrev did what 
>> it is suppose to do and showed just the diffs, but also indicated 
>> that the file was moved. Which version of webrev are you using?
>
> I'm using wevrev changeset: 24:8cd091802cd0 with Mercurial 4.5.3 on 
> Ubuntu 18.04 LTS.
>
>> What do "hg diff" and "hg status" show?
>
> For example, rename Makefile to Makefile.orig:
>
> ```
> $ hg mv Makefile Makefile.orig
> $ hg status
> A Makefile.orig
> R Makefile
This part looks correct.
> $ hg diff
> diff -r c41d1303a87c Makefile
> --- a/Makefile  Mon Nov 04 13:02:40 2019 -0800
> +++ /dev/null   Thu Jan 01 00:00:00 1970 +0000
> @@ -1,64 +0,0 @@
> -#
> -# Copyright (c) 2012, 2015, Oracle and/or its affiliates. All rights 
> reserved.
> -# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>        :
>      (snip)
> ```
This part is odd. Not sure why it says "diff -r". Mine looks like:

$ hg status
A test/hotspot/jtreg/serviceability/sa/CDSJMapClstats2.java
R test/hotspot/jtreg/serviceability/sa/CDSJMapClstats.java

$ hg diff
diff --git a/test/hotspot/jtreg/serviceability/sa/CDSJMapClstats.java 
b/test/hotspot/jtreg/serviceability/sa/CDSJMapClstats2.java
rename from test/hotspot/jtreg/serviceability/sa/CDSJMapClstats.java
rename to test/hotspot/jtreg/serviceability/sa/CDSJMapClstats2.java
--- a/test/hotspot/jtreg/serviceability/sa/CDSJMapClstats.java
+++ b/test/hotspot/jtreg/serviceability/sa/CDSJMapClstats2.java
@@ -40,7 +40,7 @@
<snip>

..and the actual diff follows the above, which is just a one line edit. 
Do you have an override of "diff" in your .hgrc?

BTW, my Mercurial is 3.6.

Chris


>
> It seems to be a problem in hg instead of webrev.
>
>
> Thanks,
>
> Yasumasa
>
>
>> thanks,
>>
>> Chris
>>>
>>>> The rest of the changes look fine.
>>>
>>> Thanks!
>>>
>>>
>>> Yasumasa
>>>
>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 11/1/19 1:56 AM, Yasumasa Suenaga wrote:
>>>>> (Changed subject to review request)
>>>>>
>>>>> Hi,
>>>>>
>>>>> I converted LinuxDebuggerLocal.c to C++ code, and it works fine on 
>>>>> submit repo.
>>>>> (mach5-one-ysuenaga-JDK-8233285-1-20191101-0746-6336009)
>>>>>
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233285/webrev.00/
>>>>>
>>>>> Could you review it?
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2019/11/01 8:54, Yasumasa Suenaga wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> On 2019/11/01 7:55, David Holmes wrote:
>>>>>>> Hi Yasumasa,
>>>>>>>
>>>>>>> New build dependencies cannot be added lightly. This impacts 
>>>>>>> everyone who maintains build/test farms.
>>>>>>
>>>>>> Ok, thanks for telling it.
>>>>>>
>>>>>>> We already use the C++ demangling capabilities in the VM. Is 
>>>>>>> there some way to export that for use by libsaproc ?
>>>>>>>
>>>>>>> Otherwise using C++ demangle may still be the better choice 
>>>>>>> given we already have it as a dependency.
>>>>>>
>>>>>> I found abi::__cxa_demangle() is used in ElfDecoder::demangle() 
>>>>>> at decoder_linux.cpp .
>>>>>> It is similar with my original proposal.
>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/sa-demangle/
>>>>>>
>>>>>> I agree with David to use C++ demangle way.
>>>>>> However we need to choice the fix from following:
>>>>>>
>>>>>>    A. Convert LinuxDebuggerLocal.c to C++ code
>>>>>>    B. Add C++ code for libsaproc.so to demangle symbols.
>>>>>>
>>>>>> I've discussed with Chris about it in [1].
>>>>>> Option A might be large change.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> [1] 
>>>>>> https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-October/029716.html
>>>>>>
>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>> On 1/11/2019 12:58 am, Chris Plummer wrote:
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> Here's the failure during configure:
>>>>>>>>
>>>>>>>> [2019-10-31T06:07:45,131Z] checking demangle.h usability... no
>>>>>>>> [2019-10-31T06:07:45,150Z] checking demangle.h presence... no
>>>>>>>> [2019-10-31T06:07:45,150Z] checking for demangle.h... no
>>>>>>>> [2019-10-31T06:07:45,151Z] configure: error: Could not find 
>>>>>>>> demangle.h! You might be able to fix this by running 'sudo yum 
>>>>>>>> install binutils-devel'.
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/31/19 1:08 AM, Yasumasa Suenaga wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I filed this enhancement to JBS:
>>>>>>>>>
>>>>>>>>>   https://bugs.openjdk.java.net/browse/JDK-8233285
>>>>>>>>>
>>>>>>>>> Also I pushed the changes to submit repo, but it was failed.
>>>>>>>>>
>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/bfbc49233c26
>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/430e4f65ef25
>>>>>>>>>
>>>>>>>>> According to the email from Mach 5, dependency errors were 
>>>>>>>>> occurred in jib.
>>>>>>>>> Can someone share the details?
>>>>>>>>> I'm not familiar in jib, so I want help.
>>>>>>>>>
>>>>>>>>> mach5-one-ysuenaga-JDK-8233285-20191031-0606-6301426
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2019/10/31 11:23, Chris Plummer wrote:
>>>>>>>>>> You can change the configure script. I don't know if there's 
>>>>>>>>>> any concerns with using libiberty.a. That's possibly a legal 
>>>>>>>>>> question (GNU GPL). You might want to ask that on jdk-dev 
>>>>>>>>>> and/or build-dev.
>>>>>>>>>>
>>>>>>>>>> Chris
>>>>>>>>>>
>>>>>>>>>> On 10/30/19 7:14 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>> Hi Chris,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for quick reply!
>>>>>>>>>>>
>>>>>>>>>>> If we convert LinuxDebuggerLocal.c to C++ code, we have to 
>>>>>>>>>>> convert a lot of JNI calls to C++ style.
>>>>>>>>>>> For example:
>>>>>>>>>>>
>>>>>>>>>>>   (*env)->FindClass(env, "java/lang/String")
>>>>>>>>>>>       to
>>>>>>>>>>>   env->FindClass("java/lang/String")
>>>>>>>>>>>
>>>>>>>>>>> Can it be accepted?
>>>>>>>>>>>
>>>>>>>>>>> OTOH I said in my email, to use cplus_demangle(), we need to 
>>>>>>>>>>> link libiberty.a which is provided by binutils. Thus I think 
>>>>>>>>>>> we need to check libiberty.a in configure script. Is it ok?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I prefer to use cplus_demangle() if we can change configure 
>>>>>>>>>>> script.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Yasumasa
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2019/10/31 11:03, Chris Plummer wrote:
>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>
>>>>>>>>>>>> I don't have concerns with adding C++ source to SA, but in 
>>>>>>>>>>>> order to do so you've put the new native code in its own 
>>>>>>>>>>>> file rather than in LinuxDebuggerLocal.c. I'd like to see 
>>>>>>>>>>>> that resolved. So either convert LinuxDebuggerLocal.c to 
>>>>>>>>>>>> C++, or use cplus_demangle().
>>>>>>>>>>>>
>>>>>>>>>>>> thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Chris
>>>>>>>>>>>>
>>>>>>>>>>>> On 10/30/19 6:54 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>
>>>>>>>>>>>>> I saw C++ frames in `jhsdb jstack --mixed`, and they were 
>>>>>>>>>>>>> mangled as below:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 0x00007ff255a8fa4c 
>>>>>>>>>>>>> _ZN9JavaCalls11call_helperEP9JavaValueRK12methodHandleP17JavaCallArgumentsP6Thread 
>>>>>>>>>>>>> + 0x6ac
>>>>>>>>>>>>> 0x00007ff255a8cc1d 
>>>>>>>>>>>>> _ZN9JavaCalls12call_virtualEP9JavaValueP5KlassP6SymbolS5_P17JavaCallArgumentsP6Thread 
>>>>>>>>>>>>> + 0x33d
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> We can demangle them via c++filt, but I think it is more 
>>>>>>>>>>>>> convenience if jstack can show demangling symbols.
>>>>>>>>>>>>> I think we can demangle in jstack with this patch. If it 
>>>>>>>>>>>>> is accepted, I will file it to JBS and send review request.
>>>>>>>>>>>>> What do you think?
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/sa-demangle/
>>>>>>>>>>>>>
>>>>>>>>>>>>> We can get the stack as below after applying this patch:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 0x00007ff1aba20a4c JavaCalls::call_helper(JavaValue*, 
>>>>>>>>>>>>> methodHandle const&, JavaCallArguments*, Thread*) + 0x6ac
>>>>>>>>>>>>> 0x00007ff1aba1dc1d JavaCalls::call_virtual(JavaValue*, 
>>>>>>>>>>>>> Klass*, Symbol*, Symbol*, JavaCallArguments*, Thread*) + 
>>>>>>>>>>>>> 0x33d
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I use abi::__cxa_demangle() for demangling, so this patch 
>>>>>>>>>>>>> adds C++ source to SA.
>>>>>>>>>>>>> If it is not comfortable, we can use cplus_demangle().
>>>>>>>>>>>>> But this function is provided by libiberty.a, so we need 
>>>>>>>>>>>>> to link it to libsaproc and need to check libiberty.a in 
>>>>>>>>>>>>> configure script.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>
>>>>
>>
>>

More information about the serviceability-dev mailing list