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