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

[Volker Simonis]

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