Since both Volker and Dmitry have reviewed this, no further reviews are necessary. I took a look at the changes in shared code, anyway, and it looks good.
Thanks, /Staffan > On 17 dec 2014, at 19:06, Dmitry Samersoff <dmitry.samers...@oracle.com> > wrote: > > Volker, > > The changes looks good for me and I'll sponsor the push. > > But please check, whether you need one more reviewer or not. > > -Dmitry > > On 2014-12-17 20:37, Volker Simonis wrote: >> Hi Dmitry, >> >> once again, thanks for your detailed review. You can find the new >> version of the webrev under: >> >> http://cr.openjdk.java.net/~simonis/webrevs/8049716.v2/ >> >> I've rebased it to the latest jdk9/hs-rt repository today. >> >> I hope I adressed all your concerns. Please find my additional >> comments inline below: >> >> And I still need a sponsor for pushing this reviewed change. Any volunteers:) >> >> Thank you and best regards, >> 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. >>> >> >> There's already a very detailed descrition of the whole story in >> "hotspot/src/share/vm/utilities/elfFuncDescTable.hpp" and I've added a >> reference to that file in the comment now. >> >>> 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; >>> >> >> Good point. Done as suggested by you. >> >>> >>> 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 >>> >> >> I think there's already a comment in place. The problem is that the >> debuginfo file only has an empty .opd section. So if we would use the >> debuginfo file for symbol resolution we would still need the .opd >> section from the regular library. This doesn't fit in the current >> function work flow which only uses ELF data from a single file and I >> didn’t wanted to change that. >> >> But your suggestion of using 'goto quit' is good and I've used that version. >> >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/MachineDescriptionPPC64.java >>> >>> 38: return (endian.equals("big")); >>> >>> is enough >>> >> >> I've used the even shorter version proposed by Alexander Smundak: >> >> return "big".equals(System.getProperty("sun.cpu.endian")) >> >>> >>> * >>> 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 >>> >> >>> This is part two of review. Code looks good for me, only minor nits. >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64CFrame.java: >>> >>> 41 missed space around = >>> 51 extra space between pc >>> >> >> Done. >> >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64ThreadContext.java >>> >>> >>> - no comments >>> >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/ppc64/PPC64ThreadContext.java >>> >>> 47, 48 extra space before = >>> >> >> Done. >> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64Thread.java >>> >>> 34 extra space before id >>> 42 extra space before = , >> >> Done. >> >>> it might be better to create a constant for size of integer. >>> >> >> I agree, but this code was just copied from the corresponding AMD64 >> version and as far as I can see all the other architectures do it the >> same way so I didn't change it. >> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadContext.java >>> >>> - no comments >>> >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadFactory.java >>> >>> -no comments >>> >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64Thread.java >>> >>> 43 missed space before ? >>> it might be be better to reformat this line to usual if and add comments >>> >> >> The same here - this is copied code. But I've adjusted it to at least >> match with the other platforms. >> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadContext.java >>> >>> - no comments >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadFactory.java >>> >>> - no comments >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/runtime/linux_ppc64/LinuxPPC64JavaThreadPDAccess.java >>> >>> >>> 57, 58 extra space before = >>> >>> 63 and below extra space after public >>> >>> 108 unaligned comments >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64CurrentFrameGuess.java >>> >>> 49 Formatting in PPC64Frame.java looks much better for me. >>> >>> 40 private static final boolean DEBUG; >>> 41 static { >>> 42 DEBUG = ... >>> 43 } >>> >>> >>> 60, 61 extra space before = >>> >>> 147 missed {} after if (DEBUG) >>> >> >> Done. >> >>> * agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64Frame.java >>> >>> 49 - 61, please fix extra and missed spaces >>> >>> 64,67 - extra spaces after static >>> 81 missed space after (int) >>> >>> 115,137 - I would prefer to always use {} after if >>> >>> 212 - multiple missed space before '?' >>> >>> 271 extra space before return >>> >> >> Done. >> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64JavaCallWrapper.java >>> >>> - no comments >>> >>> * >>> agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64RegisterMap.java >>> >>> - 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. > > > -- > Dmitry Samersoff > Oracle Java development team, Saint Petersburg, Russia > * I would love to change the world, but they won't give me the sources.
