code review round 0 for Full Debug Symbols on MacOS X hotspot (7165611)

Wed Oct 2 07:44:50 PDT 2013

Dan

You have my thumbs up changes look good


> -----Original Message-----
> From: David Holmes
> Sent: Tuesday, October 01, 2013 8:53 PM
> To: Daniel Daugherty
> Cc: serviceability-dev at openjdk.java.net; build-dev; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: code review round 0 for Full Debug Symbols on MacOS X hotspot (7165611)
> 
> Hi Dan,
> 
> Overall thumbs up. A couple of minor issues that need fixing. A few meta-comments (I hate
> seeing all this stuff duplicated again and again.
> 
> David
> -----
> 
> - common/autoconf/hotspot-spec.gmk.in
> 
> Seems a good simplification.
> 
> ----
> 
> - common/autoconf/jdk-options.m4
> 
> No comment.
> 
> ---
> 
> - common/makefiles/NativeCompilation.gmk
> 
> So JDK .diz support is phase 2? :)
> 
> The Windows changes here seem okay given that on windows a .debuginfo file should never be
> in the target list.
> 
> ---
> 
> - hotspot/make/Makefile
> 
> + $(EXPORT_CLIENT_DIR)/%.dSYM:
> $(MINIMAL1_BUILD_DIR)/%.dSYM
> 
> EXPORT_CLIENT_DIR should be EXPORT_MINIMAL_DIR.
> 
> For fun you can try building minimal on OSX, but I don't know how far you will get:
> 
> --with-jvm-variants=client,server,minimal1
> 
> I'll point out obvious issues with minimal VM support anyway.
> 
> ---
> 
> - hotspot/make/bsd/Makefile
> 
> No comment.
> 
> - hotspot/make/bsd/makefiles/buildtree.make
> 
> No comment.
> 
> - make/bsd/makefiles/defs.make
> 
> Note that the whole jdk6_or_earlier logic is defunct as this will never be used for jdk6 or
> earlier. But best to clean all that up at the one time.
> 
> Sadly I never found a satisfactory solution to the multiplicity and verbosity of the FDS
> messages, so OSX builds will now be afflicted by them too.
> 
>   328     else
>   329         EXPORT_LIST += $(EXPORT_MINIMAL_DIR)/libjvm.debuginfo
>   330     endif
> 
> This pre-existing minimal VM code needs to be modified to reference the dSYM directory on
> OSX as per the client/server cases.
> 
> ---
> 
> - hotspot/make/bsd/makefiles/dtrace.make
> 
> Note related to your changes but I don't think any of the "64" directory stuff has any
> meaning outside of Solaris.
> 
>   102         dsymutil $@
> 
> I think dsymutil should be assigned to a variable in the platform defs.make as with other
> tools.
> 
> Would be nice if objcopy/dsymutil could be hidden behind a single SYM_TOOL variables as well
> so there wasn't so much duplication of the process. You could have a single set of
> instructions to invoke SYM_TOOL, STRIP and ZIP (strip would be skipped of course because
> STRIP_POLICY would never be set on osx). Just wishful thinking ...
> 
> ---
> 
> - hotspot/make/bsd/makefiles/gcc.make
> 
> +     FASTDEBUG_CFLAGS += $(DEBUG_CFLAGS/$(BUILDARCH))
> 
> should be
> 
> +     FASTDEBUG_CFLAGS += $(FASTDEBUG_CFLAGS/$(BUILDARCH))
> 
> Don't we need the USE_CLANG variations here as for linux?
> 
> ---
> 
> - hotspot/make/bsd/makefiles/jsig.make
> - hotspot/make/bsd/makefiles/saproc.make
> 
> Similar comments to dtrace.make
> 
> ---
> 
> - make/bsd/makefiles/universal.gmk
> 
> I did not understand the additional logic here:
> 
>    63 # Copy built non-universal binaries in place
>    64 $(UNIVERSAL_COPY_LIST):
>    65         BUILT_COPY_FILES="`find
> $(EXPORT_JRE_LIB_DIR)/{i386,amd64}/$(subst $(EXPORT_JRE_LIB_DIR)/,,$@)
> 2>/dev/null`"; \
>    66         if [ -n "$${BUILT_COPY_FILES}" ]; then \
>    67           for i in $${BUILT_COPY_FILES}; do \
>    68             if [ -f $${i} ]; then \
>    69               $(MKDIR) -p $(shell dirname $@); \
>    70               $(CP) -R $${i} $@; \
>    71             fi; \
>    72             if [ -d $${i} ]; then \
>    73               $(MKDIR) -p $@; \
>    74             fi; \
>    75           done; \
>    76         fi
> 
> until I realized that foo.dSYM is a directory not a file! Even so don't
> you want to copy the contents of the directory across ?
> 
> BTW: UNIVERSAL_COPY_LIST doesn't handle minimal VM.
> 
> ---
> 
> - make/bsd/makefiles/vm.make
> 
> Similar comments to dtrace.make ref dsymutil.
> 
> ---
> 
> - hotspot/make/defs.make
> 
> No comment.
> 
> ---
> 
> - jdk/make/common/Defs-macosx.gmk
> 
> No comment
> 
> - jdk/makefiles/Tools.gmk
> 
> Not sure about this one. Logically is seems correct but up to now this
> has been defined for everything without it seeming to cause a problem.
> So why do we need to change it and what impact will it have?
> 
> David
> -----
> 
> On 21/09/2013 1:36 PM, Daniel D. Daugherty wrote:
> > Greetings,
> >
> > I have the initial support for Full Debug Symbols (FDS) on MacOS X done
> > and ready for review:
> >
> >      7165611 implement Full Debug Symbols on MacOS X hotspot
> >      https://bugs.openjdk.java.net/browse/JDK-7165611
> >
> > Here is the JDK8/HSX-25 webrev URL:
> >
> > OpenJDK:
> > http://cr.openjdk.java.net/~dcubed/fds_revamp/7165611-webrev/0-jdk8/
> > Internal:
> > http://javaweb.us.oracle.com/~ddaugher/fds_revamp/7165611-webrev/0-jdk8/
> >
> > This webrev includes changes for the follow repos:
> >
> >      jdk8
> >      jdk8/hotspot
> >      jdk8/jdk
> >      jdk8/jdk/make/closed
> >
> > Once these changes are approved, I'm planning to push them to
> > RT_Baseline. From there, they can follow the normal path to
> > Main_Baseline and eventually JDK8.
> >
> > This work enables FDS on MacOS X for the 'hotspot' repo; the changes in
> > the other repos are necessary to support importing the .diz files from
> > the MacOS X 'hotspot' build into the forest build. I also fixed a few
> > FDS related errors in the magic incantations for the new build. This is
> > mostly a port from Linux -> MacOS X/BSD with the dtrace changes ported
> > from Solaris. In other words, this is Frankenstein's monster...
> >
> > Thanks to Staffan Larsen for providing an initial set of changes
> > which I morphed into what you see here.
> >
> > Testing:
> > - JPRT HSX build and test on all platforms; verification of .diz
> >    files in the MacOS X JPRT bundles
> > - JPRT JDK8 forest build and test on all platforms; verification of
> >    .diz files in the MacOS X JPRT bundles
> >    Note: In previous FDS changesets, I also did a standalone 'jdk'
> >    repo build and test, but that no longer seems to work.
> >
> > As always, comments, questions and suggestions are welcome.
> >
> > Dan