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

Fri Oct 4 02:10:37 UTC 2013

On 3/10/2013 1:32 AM, Daniel D. Daugherty wrote:
> 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.

Don't bother with the minimal VM build attempt, you won't even get 
started. The files don't exist for BSD.

David

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