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

[Volker Simonis]

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