RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
Dmitry Samersoff
dmitry.samersoff at oracle.com
Mon Dec 15 14:32:10 UTC 2014
Volker,
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
*
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
>>>>>>
>>>>>
>>>>
>>>
--
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