RFR(S): 8049715: PPC64: First steps to enable SA on Linux/PPC64

David Holmes david.holmes at oracle.com
Mon Jul 14 22:56:42 UTC 2014 

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
>> <david.holmes at oracle.com> 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 <david.holmes at oracle.com>
>>>> 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
>>>>>> <david.holmes at oracle.com>
>>>>>> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>

More information about the serviceability-dev mailing list