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

[David Holmes]

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