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

Thu Oct 10 09:09:20 UTC 2013

I'm fine with not adding untestable minimal VM support.

Thanks,
David

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