RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
Maynard Johnson
maynardj at us.ibm.com
Tue Dec 16 16:47:59 UTC 2014
On 12/15/2014 08:32 AM, Dmitry Samersoff wrote:
> Volker,
>
> This is part two of review. Code looks good for me, only minor nits.
Thanks for the review, Dmitry.
*Volker*, will you be making the suggested changes (from all review comments) to the patch
set or do you want me to do it? Actually, I am retiring from IBM and, as of Friday, will no
longer have access to a ppc64 machine or to my IBM email account. So my colleague,
Tiago Daitx (on cc) will take over for me any responsibilities left in pushing this patch set
upstream.
Thanks again for all the help.
-Maynard
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64CFrame.java:
>
> 41 missed space around =
> 51 extra space between pc
>
>
> *
> 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 =
>
> *
> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64Thread.java
>
> 34 extra space before id
> 42 extra space before = ,
> it might be better to create a constant for size of integer.
>
> *
> 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
>
> *
> 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)
>
> * 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
>
> *
> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>
>
More information about the serviceability-dev
mailing list