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

Fri Nov 1 22:07:20 UTC 2019

Hi  Yasumasa,

I can't comment on the build changes. I don't know how the build works 
well enough.

LinuxDebuggerLocal.cpp should show up as a rename + diffs, not as a new 
file.

The rest of the changes look fine.

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