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

David Holmes david.holmes at oracle.com
Fri Jul 11 12:02:49 UTC 2014 

I'll test it out with my local custom changes while we wait for a second 
reviewer.

I plan to push to hs-rt repo.

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