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

[Dmitry Samersoff]

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.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.