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

David Holmes david.holmes at oracle.com
Mon Jul 14 11:09:31 UTC 2014 

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