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