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

Tue Nov 5 04:50:03 UTC 2019

On 11/4/19 8:47 PM, Chris Plummer wrote:
> 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?
I should have mention what's in my .hgrc. because I just noticed something:

diff =
[diff]
git=1
nodates=1

Don't ask me why I have this. I cloned someones .hgrc when openjdk first 
moved to Mercurial, and have never touched this part of it. At the very 
least the "git=1" would explain why my diff output says "diff --git ...".

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