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.
