RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
Volker Simonis
volker.simonis at gmail.com
Thu Dec 18 09:38:38 UTC 2014
Great Staffan!
Thanks a lot,
Volker
On Thu, Dec 18, 2014 at 10:31 AM, Staffan Larsen
<staffan.larsen at oracle.com> wrote:
> Since both Volker and Dmitry have reviewed this, no further reviews are necessary. I took a look at the changes in shared code, anyway, and it looks good.
>
> Thanks,
> /Staffan
>
>> On 17 dec 2014, at 19:06, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
>>
>> Volker,
>>
>> The changes looks good for me and I'll sponsor the push.
>>
>> But please check, whether you need one more reviewer or not.
>>
>> -Dmitry
>>
>> On 2014-12-17 20:37, Volker Simonis wrote:
>>> Hi Dmitry,
>>>
>>> once again, thanks for your detailed review. You can find the new
>>> version of the webrev under:
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/8049716.v2/
>>>
>>> I've rebased it to the latest jdk9/hs-rt repository today.
>>>
>>> I hope I adressed all your concerns. Please find my additional
>>> comments inline below:
>>>
>>> And I still need a sponsor for pushing this reviewed change. Any volunteers:)
>>>
>>> Thank you and best regards,
>>> Volker
>>>
>>>> Below is fist part of review - shared files.
>>>>
>>>>
>>>> * agent/make/Makefile - no comments
>>>>
>>>> * agent/src/os/linux/LinuxDebuggerLocal.c - no comments
>>>>
>>>> * agent/src/os/linux/symtab.c:
>>>>
>>>> 438:
>>>> - What is the magic of symbols that starts with '.' ?
>>>> - As far as I understand you are getting indirect value from opd section.
>>>>
>>>
>>> There's already a very detailed descrition of the whole story in
>>> "hotspot/src/share/vm/utilities/elfFuncDescTable.hpp" and I've added a
>>> reference to that file in the comment now.
>>>
>>>> Could you reformat it a bit to have it better readable?
>>>>
>>>> Something like:
>>>>
>>>> uintptr_t sym_value;
>>>> ...
>>>> symvalue = syms->st_value
>>>>
>>>> #ifdef ppc64
>>>> // Some comments here
>>>> // ppc specific code here
>>>> sym_value =
>>>> #endif
>>>>
>>>> symtab->symbols[j].offset = sym_value - baseaddr;
>>>>
>>>
>>> Good point. Done as suggested by you.
>>>
>>>>
>>>> 454:
>>>>
>>>> I appreciate detailed comments here.
>>>>
>>>> if (false) can cause unreachable code warning, and unused variable
>>>> warning so it might be better to add #ifdef ppc64 on caller
>>>> site at ll. 516 and leave here only a comment.
>>>>
>>>> But if you decide to guard against try_debuginfo please replace
>>>>
>>>> if (false)
>>>>
>>>> to
>>>>
>>>> goto quit
>>>>
>>>
>>> I think there's already a comment in place. The problem is that the
>>> debuginfo file only has an empty .opd section. So if we would use the
>>> debuginfo file for symbol resolution we would still need the .opd
>>> section from the regular library. This doesn't fit in the current
>>> function work flow which only uses ELF data from a single file and I
>>> didn’t wanted to change that.
>>>
>>> But your suggestion of using 'goto quit' is good and I've used that version.
>>>
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/MachineDescriptionPPC64.java
>>>>
>>>> 38: return (endian.equals("big"));
>>>>
>>>> is enough
>>>>
>>>
>>> I've used the even shorter version proposed by Alexander Smundak:
>>>
>>> return "big".equals(System.getProperty("sun.cpu.endian"))
>>>
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxCDebugger.java
>>>> - no comments
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxThreadContextFactory.java
>>>> - no comments
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ProcDebuggerLocal.java
>>>> - no comments
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/RemoteDebuggerClient.java
>>>> - no comments
>>>>
>>>> * agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java - no comments
>>>>
>>>> * agent/src/share/classes/sun/jvm/hotspot/runtime/VFrame.java - no comments
>>>>
>>>> * make/linux/makefiles/sa.make - no comments
>>>>
>>>> * make/sa.files - no comments
>>>>
>>>> * src/share/vm/runtime/vmStructs.cpp
>>>> - no comments
>>>>
>>>
>>>> This is part two of review. Code looks good for me, only minor nits.
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64CFrame.java:
>>>>
>>>> 41 missed space around =
>>>> 51 extra space between pc
>>>>
>>>
>>> Done.
>>>
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64ThreadContext.java
>>>>
>>>>
>>>> - no comments
>>>>
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/ppc64/PPC64ThreadContext.java
>>>>
>>>> 47, 48 extra space before =
>>>>
>>>
>>> Done.
>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64Thread.java
>>>>
>>>> 34 extra space before id
>>>> 42 extra space before = ,
>>>
>>> Done.
>>>
>>>> it might be better to create a constant for size of integer.
>>>>
>>>
>>> I agree, but this code was just copied from the corresponding AMD64
>>> version and as far as I can see all the other architectures do it the
>>> same way so I didn't change it.
>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadContext.java
>>>>
>>>> - no comments
>>>>
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadFactory.java
>>>>
>>>> -no comments
>>>>
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64Thread.java
>>>>
>>>> 43 missed space before ?
>>>> it might be be better to reformat this line to usual if and add comments
>>>>
>>>
>>> The same here - this is copied code. But I've adjusted it to at least
>>> match with the other platforms.
>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadContext.java
>>>>
>>>> - no comments
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadFactory.java
>>>>
>>>> - no comments
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/runtime/linux_ppc64/LinuxPPC64JavaThreadPDAccess.java
>>>>
>>>>
>>>> 57, 58 extra space before =
>>>>
>>>> 63 and below extra space after public
>>>>
>>>> 108 unaligned comments
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64CurrentFrameGuess.java
>>>>
>>>> 49 Formatting in PPC64Frame.java looks much better for me.
>>>>
>>>> 40 private static final boolean DEBUG;
>>>> 41 static {
>>>> 42 DEBUG = ...
>>>> 43 }
>>>>
>>>>
>>>> 60, 61 extra space before =
>>>>
>>>> 147 missed {} after if (DEBUG)
>>>>
>>>
>>> Done.
>>>
>>>> * agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64Frame.java
>>>>
>>>> 49 - 61, please fix extra and missed spaces
>>>>
>>>> 64,67 - extra spaces after static
>>>> 81 missed space after (int)
>>>>
>>>> 115,137 - I would prefer to always use {} after if
>>>>
>>>> 212 - multiple missed space before '?'
>>>>
>>>> 271 extra space before return
>>>>
>>>
>>> Done.
>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64JavaCallWrapper.java
>>>>
>>>> - no comments
>>>>
>>>> *
>>>> agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64RegisterMap.java
>>>>
>>>> - no comments
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>> On 2014-12-09 21:10, Volker Simonis wrote:
>>>>> Hi,
>>>>>
>>>>> can somebody from the serviceability team please review this webrev?
>>>>>
>>>>> http://cr.openjdk.java.net/~simonis/webrevs/8049716
>>>>> https://bugs.openjdk.java.net/browse/JDK-8049716
>>>>>
>>>>> The shared changes are really all trivial.
>>>>>
>>>>> Thanks,
>>>>> Volker
>>>>>
>>>>>
>>>>> On Fri, Dec 5, 2014 at 4:01 PM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>>>>> Hi Sasha,
>>>>>>
>>>>>> thanks for looking at this change.
>>>>>> I'll incorporate your suggestions into the final version.
>>>>>> I'm just waiting for one more review before preparing a new webrev.
>>>>>>
>>>>>> Regards,
>>>>>> Volker
>>>>>>
>>>>>>
>>>>>> On Fri, Dec 5, 2014 at 3:10 PM, Maynard Johnson <maynardj at us.ibm.com> wrote:
>>>>>>> On 12/04/2014 07:50 PM, Alexander Smundak wrote:
>>>>>>>> You are correct, but there no need to have this code for LE at all.
>>>>>>> Agreed. I'm fine with adding the "&& !defined(ABI_ELFv2)" throughout that file
>>>>>>> along with the "#if defined(ppc64)".
>>>>>>>>
>>>>>>>> BTW, a bit on nitpicking in the same file:
>>>>>>>> + String endian = System.getProperty("sun.cpu.endian");
>>>>>>>> + if (endian.equals("big"))
>>>>>>>> + return true;
>>>>>>>> + else
>>>>>>>> + return false;
>>>>>>>> can be rewirtten as
>>>>>>>> return "big".equals(System.getProperty("sun.cpu.endian"));
>>>>>>> Right. A silly piece of coding there. :-/
>>>>>>>
>>>>>>> -Maynard
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, Dec 4, 2014 at 3:43 PM, Maynard Johnson <maynardj at us.ibm.com> wrote:
>>>>>>>>> On 12/04/2014 01:15 PM, Alexander Smundak wrote:
>>>>>>>>>> The changes for agent/src/os/linux/symtab.c
>>>>>>>>>> b/agent/src/os/linux/symtab.c in
>>>>>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/8049716 will break
>>>>>>>>>> Linux/PPC64 little-endian:
>>>>>>>>>> it uses ABIv2, which dropped function descriptors. So the preprocessor
>>>>>>>>>> brackets in it should
>>>>>>>>>> read
>>>>>>>>>> #if defined(ppc64) && !defined(ABI_ELFv2)
>>>>>>>>>> instead of just
>>>>>>>>>> #if defined(ppc64)
>>>>>>>>> Hi, Alexander,
>>>>>>>>> I think this is actually fine everywhere except one place. The 'opd' variable will be
>>>>>>>>> set to something other than NULL at line 379 only if running on ppc64 BE. So in
>>>>>>>>> the rest of that function, opd is checked for non-null before using it. The only
>>>>>>>>> place where I think there may be a problem is at line 455:
>>>>>>>>>
>>>>>>>>> --------------------------
>>>>>>>>> #if defined(ppc64)
>>>>>>>>> // On Linux/PPC64 the debuginfo files contain an empty file descriptor
>>>>>>>>> // section (i.e. '.opd' section) which makes the resolution of symbols
>>>>>>>>> // with the above algorithm impossible (we would need the have both, the
>>>>>>>>> // .opd section from the library and the symbol table from the debuginfo
>>>>>>>>> // file which doesn't match with the current workflow.)
>>>>>>>>> if (false) {
>>>>>>>>> #else
>>>>>>>>> // Look for a separate debuginfo file.
>>>>>>>>> if (try_debuginfo) {
>>>>>>>>> #endif
>>>>>>>>> --------------------------
>>>>>>>>>
>>>>>>>>> Here I think we should do as you suggest:
>>>>>>>>> #if defined(ppc64) && !defined(ABI_ELFv2)
>>>>>>>>>
>>>>>>>>> -Maynard
>>>>>>>>>>
>>>>>>>>>> Sorry for the late notice.
>>>>>>>>>> Sasha
>>>>>>>>>>
>>>>>>>>>> On Thu, Dec 4, 2014 at 9:50 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> I'd like to submit this webrev which adds support for the SA agent on
>>>>>>>>>>> Linux/PPC64 on behalf of Maynard Johnson who is the main author of the
>>>>>>>>>>> change:
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/8049716
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8049716
>>>>>>>>>>>
>>>>>>>>>>> I have already reviewed and tested the change and from my side
>>>>>>>>>>> everything looks fine.
>>>>>>>>>>>
>>>>>>>>>>> The change touches quite some shared code but all of these changes are
>>>>>>>>>>> trivial and straight-forward (i.e. they just add Linux/PPC64 support
>>>>>>>>>>> with the help of '#ifdef's in C or yet another 'elseif' clause in
>>>>>>>>>>> Java).
>>>>>>>>>>>
>>>>>>>>>>> We need a second reviewer and a sponsor who can push this to the
>>>>>>>>>>> hotspot repository once the review is completed.
>>>>>>>>>>>
>>>>>>>>>>> Thank you and best regards,
>>>>>>>>>>> Volker
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>
>>>>
>>>> --
>>>> Dmitry Samersoff
>>>> Oracle Java development team, Saint Petersburg, Russia
>>>> * I would love to change the world, but they won't give me the sources.
>>
>>
>> --
>> Dmitry Samersoff
>> Oracle Java development team, Saint Petersburg, Russia
>> * I would love to change the world, but they won't give me the sources.
>
More information about the serviceability-dev
mailing list