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