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

[Daniel D. Daugherty]

Sorry for the delay in getting back to this. I've been side tracked by
other tasks.

I've made all the changes needed to address most of the comments
below, but I still have to double check and test...


On 10/1/13 8:52 PM, David Holmes wrote:
> Hi Dan,
>
> Overall thumbs up.

Thanks!


> A couple of minor issues that need fixing.

Will address those below...


> A few meta-comments (I hate seeing all this stuff duplicated again and 
> again.

Agreed that the duplication is bad (as before), but there is some
hope that this stuff will get refactored...


>
> 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? :)

Yes, phase 2 if someone gets to it (probably me again).


> The Windows changes here seem okay given that on windows a .debuginfo 
> file should never be in the target list.

Right, I happened to notice that the .debuginfo file logic was not
being excluded on Windows. Yes, this is the MacOS X bug, but I figured
I would clean it up while I was there.


> ---
>
> - 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.

Since you pointed out in a later email that minimal1 isn't
supported on MacOS X, I'm going to drop those changes. I'm
going to leave the Client VM support since there are folks
out in OpenJDK trying to get the Client VM working on MacOS X.

It does look like someone added minimal1 support for other
non-Linux platforms, but I'm not planning to clean that up.


> ---
>
> - 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.

Agreed that we (Oracle) currently don't have plans to drop HSX-25 into
JDK6 or OpenJDK6, but I don't want to rule out future insanity.

Also agreed that cleaning up that logic should all be done at the same
time using a different bug ID.


> 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.

Also agreed, I've read your emails on that topic a few times, but
nothing springs to mind...


>
>  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.

Since MacOS X doesn't support the minimal1 config, I've dropped that
block of logic. Perhaps another bug should be used to remove all
mention of minimal1 config from the MacOS X/BSD makefiles.


> ---
>
> - 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.

And now that 32-bit Solaris support is being dropped, this whole '64'
subdir stuff might become obsolete/moot.


> 102         dsymutil $@
>
> I think dsymutil should be assigned to a variable in the platform 
> defs.make as with other tools.

Good idea. It surprised me when the original Makefiles had this
non-macro'ized call to dsymutil, but I went ahead and simply
refactored the existing call (and copied it).

I'll figure out how/where to fix it.


> 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 ...

Could/should be done when the other refactoring is done to remove
duplication.


>
> ---
>
> - hotspot/make/bsd/makefiles/gcc.make
>
> +     FASTDEBUG_CFLAGS += $(DEBUG_CFLAGS/$(BUILDARCH))
>
> should be
>
> +     FASTDEBUG_CFLAGS += $(FASTDEBUG_CFLAGS/$(BUILDARCH))

Thanks for catching the cut-n-paste typo...


>
> Don't we need the USE_CLANG variations here as for linux?

I suppose. I was trying to ignore CLANG, but I'll go ahead and
drop in the changes (untested).


> ---
>
> - hotspot/make/bsd/makefiles/jsig.make
> - hotspot/make/bsd/makefiles/saproc.make
>
> Similar comments to dtrace.make

Fixed the literal 'dsymutil' usages.


>
> ---
>
> - 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 ?

BUILT_COPY_FILES includes both files and directories so everything
should get copied across. I've added some commments to the rule
header and reorganized the logic a bit to hopefully be more clear.


> BTW: UNIVERSAL_COPY_LIST doesn't handle minimal VM.

That's a good thing. :-)


>
> ---
>
> - make/bsd/makefiles/vm.make
>
> Similar comments to dtrace.make ref dsymutil.

Fixed the literal 'dsymutil' usages.


>
> ---
>
> - 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?

It just seemed wrong to define something that should never be used
on non-Solaris platforms. My control build of the entire forest
didn't have an issue with this change so I'm planning to keep it.

Dan



>
> 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