RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64

Volker Simonis volker.simonis at gmail.com
Wed Dec 17 17:37:13 UTC 2014 

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.

More information about the serviceability-dev mailing list