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