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

[David Holmes]

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?

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