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

Mon Jul 14 09:44:51 UTC 2014

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/

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