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.
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;
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
*
agent/src/share/classes/sun/jvm/hotspot/debugger/MachineDescriptionPPC64.java
38: return (endian.equals("big"));
is enough
*
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
-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.simo...@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 <mayna...@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 <mayna...@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.simo...@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.
