RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
Staffan Larsen
staffan.larsen at oracle.com
Thu Dec 18 09:31:31 UTC 2014
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