[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
From: Volker Simonis <volker.simonis () gmail ! com>
Date: 2014-12-18 9:38:38
Message-ID: CA+3eh12GV3otdHSFiPqKQdePaOGV0=p5HgfByYCnzgBKwYHd2Q () mail ! gmail ! com
[Download RAW message or body]
Great Staffan!
Thanks a lot,
Volker
On Thu, Dec 18, 2014 at 10:31 AM, Staffan Larsen
<staffan.larsen@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.samersoff@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.simonis@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 <maynardj@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 <maynardj@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.simonis@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.
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic