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

Mon Nov 4 17:28:08 UTC 2019

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? What do "hg diff" 
and "hg status" show?

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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>
>>