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

Wed Oct 9 15:03:01 UTC 2013

Replies also inline...

On 10/9/13 6:02 AM, David Holmes wrote:
> inline ...
>
> On 9/10/2013 8:59 AM, Daniel D. Daugherty wrote:
>> On 10/1/13 8:52 PM, David Holmes wrote:
>>> - 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.
>
> Yes I did that when adding minimal support. Seemed better than leaving 
> someone to have to rediscover what needed to be implemented if/when 
> minimal support was expanded.

Thanks for the history. Are you still OK if I leave out the
FDS Minimal1 support?

> Someday I hope to remove all the duplicated EXPORT_* stuff and 
> generate it based on the requested JDK_VARIANT_* values. And do it in 
> a platform indpendent way - once! :)

Well it can be mostly platform independent, but there will be
minor differences in what is exported by the different platforms.

>
>>> 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.
>
> I do! We need to cut ties with historical baggage.

Good sentiment, but not for this changeset. I'll file a bug to track
your idea of "best to clean all that up at one time".

>
>>> - 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.
>
> Doesn't this assume that the directory will appear before the files 
> within it? Is that guaranteed?

The way find works is that it lists the directory prior to listing
the files within the directory. However, even if find didn't, the
containing directory would be created via line 69 above. The one
non-obvious feature of lines 72-74 is that an empty directory named
in the BUILD_COPY_FILES list would get 'copied' to the destination.

Please check out the latest version when I get it out for review.

> Which leads me to ask why we have cp -R for copying files?

MacOS X strongly discourages use of 'cp -r' and recommends 'cp -R'
instead because 'cp -R' properly copies non-directory and non-file
file system objects, e.g., symlinks. So if BUILD_COPY_FILES happens
to contain a symlink, then the symlink is copied to the destination.

>
>>> - 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.
>
> Ok.
>
> Thanks,
> David

Again, thanks for the thorough reviews.

Dan

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