David,
Thanks for the thorough review (as always)!
It will take me a while to process all the comments, but another
reply will be forthcoming.
Dan
On 10/1/13 8:52 PM, David Holmes wrote:
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
