Great Staffan! Thanks a lot, Volker
On Thu, Dec 18, 2014 at 10:31 AM, Staffan Larsen <staffan.lar...@oracle.com> wrote: > 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. >
