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

Fri Jul 11 11:54:24 UTC 2014

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