Great! Thanks a lot, Volker
On Tue, Jul 15, 2014 at 12:56 AM, David Holmes <[email protected]> wrote: > All changes (hotspot and top-level) are now in the jdk9/hs-rt forest. > > David > > > On 14/07/2014 9:09 PM, David Holmes wrote: >> >> On 14/07/2014 7:44 PM, Volker Simonis wrote: >>> >>> On Sun, Jul 13, 2014 at 11:22 PM, David Holmes >>> <[email protected]> wrote: >>>> >>>> Hi Volker, >>>> >>>> Just discovered you didn't quite pick up on all of my change - the >>>> ARM entry >>>> is to be deleted. Only the open platforms need to be listed: >>>> >>>> >>>>>> # No SA Support for IA64 or zero >>>>>> ADD_SA_BINARIES/ia64 = >>>>>> ADD_SA_BINARIES/zero = >>>> >>>> >>> >>> OK, but then I also remove IA64 as it isn't an open platform either: >>> >>> http://cr.openjdk.java.net/~simonis/webrevs/8049715.v4/ >> >> >> Yes good point. ia64 should be eradicated from the build system :) >> >> I will put this altogether in the AM. >> >>> I've also added Vladimir as reviewer. >> >> >> Great >> >> Thanks, >> David >> >> >>> Thank you and best regards, >>> Volker >>> >>> >>>> Thanks, >>>> David >>>> >>>> On 11/07/2014 9:54 PM, Volker Simonis wrote: >>>>> >>>>> >>>>> On Fri, Jul 11, 2014 at 6:36 AM, David Holmes <[email protected]> >>>>> wrote: >>>>>> >>>>>> >>>>>> Hi Volker, >>>>>> >>>>>> >>>>>> On 10/07/2014 8:12 PM, Volker Simonis wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi David, >>>>>>> >>>>>>> thanks for looking at this. Here's my new version of the change with >>>>>>> some of your suggestions applied: >>>>>>> >>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/8049715.v2 >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> I have a simpler counter proposal (also default -> DEFAULT as that >>>>>> seems >>>>>> to >>>>>> be the style): >>>>>> >>>>>> # Serviceability Binaries >>>>>> >>>>>> ADD_SA_BINARIES/DEFAULT = >>>>>> $(EXPORT_JRE_LIB_ARCH_DIR)/libsaproc.$(LIBRARY_SUFFIX) \ >>>>>> >>>>>> $(EXPORT_LIB_DIR)/sa-jdi.jar >>>>>> >>>>>> ifeq ($(ENABLE_FULL_DEBUG_SYMBOLS),1) >>>>>> ifeq ($(ZIP_DEBUGINFO_FILES),1) >>>>>> ADD_SA_BINARIES/DEFAULT += >>>>>> $(EXPORT_JRE_LIB_ARCH_DIR)/libsaproc.diz >>>>>> else >>>>>> ADD_SA_BINARIES/DEFAULT += >>>>>> $(EXPORT_JRE_LIB_ARCH_DIR)/libsaproc.debuginfo >>>>>> endif >>>>>> endif >>>>>> >>>>>> ADD_SA_BINARIES/$(HS_ARCH) = $(ADD_SA_BINARIES/DEFAULT) >>>>>> >>>>>> >>>>>> # No SA Support for IA64 or zero >>>>>> ADD_SA_BINARIES/ia64 = >>>>>> ADD_SA_BINARIES/zero = >>>>>> >>>>>> --- >>>>>> >>>>>> The open logic only has to worry about open platforms. The custom >>>>>> makefile >>>>>> can accept the default or override as it desires. >>>>>> >>>>>> I thought about conditionally setting ADD_SA_BINARIES/$(HS_ARCH) >>>>>> but the >>>>>> above is simple and clear. >>>>>> >>>>>> Ok? >>>>>> >>>>> >>>>> Perfect! >>>>> >>>>> Here's the new webrev with your proposed changes (tested on >>>>> Linux/x86_64 and ppc64): >>>>> >>>>> http://cr.openjdk.java.net/~simonis/webrevs/8049715.v3 >>>>> >>>>> Thanks for sponsoring, >>>>> Volker >>>>> >>>>>> I'll sponsor this one of course (so its safe for other reviewers to >>>>>> jump >>>>>> in >>>>>> now :) ). >>>>>> >>>>>> Thanks, >>>>>> David >>>>>> >>>>>> >>>>>> >>>>>>> Please find more information inline: >>>>>>> >>>>>>> On Thu, Jul 10, 2014 at 4:41 AM, David Holmes >>>>>>> <[email protected]> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Hi Volker, >>>>>>>> >>>>>>>> Comments below where you might expect them :) >>>>>>>> >>>>>>>> >>>>>>>> On 10/07/2014 3:36 AM, Volker Simonis wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> could someone please review and sponsor the following change which >>>>>>>>> does some preliminary work for enabling the SA agent on >>>>>>>>> Linux/PPC64: >>>>>>>>> >>>>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/8049715/ >>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8049715 >>>>>>>>> >>>>>>>>> Details: >>>>>>>>> >>>>>>>>> Currently, we don't support the SA agent on Linux/PPC64. This >>>>>>>>> change >>>>>>>>> fixes the buildsystem such that the SA libraries (i.e. libsaproc.so >>>>>>>>> and sa-jdi.jar) will be correctly build and copied into the >>>>>>>>> resulting >>>>>>>>> jdk images. >>>>>>>>> >>>>>>>>> This change also contains some small fixes in sa-jdi.jar to >>>>>>>>> correctly >>>>>>>>> detect Linux/PPC64 as supported SA platform. (The actual >>>>>>>>> implementation of the Linux/PPC64 specific code will be handled by >>>>>>>>> "8049716 PPC64: Implement SA on Linux/PPC64" - >>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8049716). >>>>>>>>> >>>>>>>>> One thing which require special attention are the changes in >>>>>>>>> make/linux/makefiles/defs.make which may touch the closed ppc >>>>>>>>> port. In >>>>>>>>> my change I've simply added 'ppc' to the list of supported >>>>>>>>> architectures, but this may break the 32-bit ppc build. I think the >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> It wouldn't break it but I was expecting to see ppc64 here. >>>>>>>> >>>>>>> >>>>>>> The problem is that currently the decision if the SA agent will be >>>>>>> build is based on the value of HS_ARCH. But HS_ARCH is the 'basic >>>>>>> architecture' (i.e. x86 or sparc) so there's no easy way to choose >>>>>>> the >>>>>>> SA agent for only a 64-bit platform (like ppc64 or amd64) and not for >>>>>>> its 32-bit counterpart (i.e. i386 or ppc). >>>>>>> >>>>>>> The only possibility with the current solution would be to only >>>>>>> conditionally set ADD_SA_BINARIES/ppc if ARCH_DATA_MODEL is 64. But >>>>>>> that wouldn't make the code nicer either:) >>>>>>> >>>>>>>> >>>>>>>>> current code is to verbose and error prone anyway. It would be >>>>>>>>> better >>>>>>>>> to have something like: >>>>>>>>> >>>>>>>>> ADD_SA_BINARIES = >>>>>>>>> $(EXPORT_JRE_LIB_ARCH_DIR)/libsaproc.$(LIBRARY_SUFFIX) >>>>>>>>> $(EXPORT_LIB_DIR)/sa-jdi.jar >>>>>>>>> >>>>>>>>> ifeq ($(ENABLE_FULL_DEBUG_SYMBOLS),1) >>>>>>>>> ifeq ($(ZIP_DEBUGINFO_FILES),1) >>>>>>>>> ADD_SA_BINARIES += >>>>>>>>> $(EXPORT_JRE_LIB_ARCH_DIR)/libsaproc.diz >>>>>>>>> else >>>>>>>>> ADD_SA_BINARIES += >>>>>>>>> $(EXPORT_JRE_LIB_ARCH_DIR)/libsaproc.debuginfo >>>>>>>>> endif >>>>>>>>> endif >>>>>>>>> >>>>>>>>> ifneq (,$(findstring $(ARCH), amd64 x86_64 i686 i586 sparc sparcv9 >>>>>>>>> ppc64)) >>>>>>>>> EXPORT_LIST += $(ADD_SA_BINARIES/$(HS_ARCH)) >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> You wouldn't need/want the $(HS_ARCH) there. >>>>>>>> >>>>>>> >>>>>>> Sorry, that was a type of course. It should read: >>>>>>> >>>>>>> ifneq (,$(findstring $(ARCH), amd64 x86_64 i686 i586 sparc >>>>>>> sparcv9 >>>>>>> ppc64)) >>>>>>> EXPORT_LIST += $(ADD_SA_BINARIES) >>>>>>> >>>>>>> But that's not necessary now anymore (see new version below). >>>>>>> >>>>>>>> >>>>>>>>> endif >>>>>>>>> >>>>>>>>> With this solution we only define ADD_SA_BINARIES once (because the >>>>>>>>> various definitions for the different platforms are equal >>>>>>>>> anyway). But >>>>>>>>> again this may affect other closed ports so please advise which >>>>>>>>> solution you'd prefer. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> The above is problematic for customizations. An alternative would >>>>>>>> be to >>>>>>>> set >>>>>>>> ADD_SA_BINARIES/default once with all the file names. Then: >>>>>>>> >>>>>>>> ADD_SA_BINARIES/$(ARCH) = $(ADD_SA_BINARIES/default) >>>>>>>> # No SA Support for IA64 or zero >>>>>>>> ifneq (, $(findstring $(ARCH), ia64, zero)) >>>>>>>> ADD_SA_BINARIES/$(ARCH) = >>>>>>>> >>>>>>>> Each ARCH handled elsewhere would then still set >>>>>>>> ADD_SA_BINARIES/$(ARCH) >>>>>>>> if >>>>>>>> needed. >>>>>>>> >>>>>>>> Does that seem reasonable? >>>>>>>> >>>>>>> >>>>>>> The problem with using ARCH is that it is not "reliable" in the sens >>>>>>> that its value differs for top-level and hotspot-only makes. See >>>>>>> "8046471: Use OPENJDK_TARGET_CPU_ARCH instead of legacy value for >>>>>>> hotspot ARCH" and my fix "8048232: Fix for 8046471 breaks PPC64 >>>>>>> build". >>>>>>> >>>>>>> But using ADD_SA_BINARIES/default to save redundant lines is a good >>>>>>> idea. I've updated the patch accordingly and think that the new >>>>>>> solution is a good compromise between readability and not touching >>>>>>> existing/closed part. >>>>>>> >>>>>>> Are you fine with the new version at >>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/8049715.v2 ? >>>>>>> >>>>>>>> >>>>>>>>> Notice that this change also requires a tiny fix in the top-level >>>>>>>>> repository which must be pushed AFTER this change. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Can you elaborate please? >>>>>>>> >>>>>>> >>>>>>> I've also submitted the corresponding top-level repository change for >>>>>>> review which expects to find the SA agent libraries on Linux/ppc64 in >>>>>>> order to copy them into the image directory: >>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/8049715_top_level/ >>>>>>> >>>>>>> But once that will be pushed, the build will fail if these HS changes >>>>>>> will not be in place to actually build the libraries. >>>>>>> >>>>>>>> Thanks, >>>>>>>> David >>>>>>>> >>>>>>>> >>>>>>>>> Thank you and best regards, >>>>>>>>> Volker >>>>>>>>> >>>>>>>> >>>>>> >>>> >
