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-...@openjdk.java.net; build-dev; > hotspot-runtime-...@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