RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
Dmitry Samersoff
dmitry.samersoff at oracle.com
Wed Dec 17 18:06:20 UTC 2014
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