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/ I've also added Vladimir as reviewer. 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 >>>>>> >>>>> >>> >
