[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(S): 8049715: PPC64: First steps to enable SA on Linux/PPC64
From: Volker Simonis <volker.simonis () gmail ! com>
Date: 2014-07-15 7:37:12
Message-ID: CA+3eh13PTuP=yBuwKetU6ymeuMSni-cO1o6o7enYkcnUu0QgRQ () mail ! gmail ! com
[Download RAW message or body]
Great!
Thanks a lot,
Volker
On Tue, Jul 15, 2014 at 12:56 AM, David Holmes <david.holmes@oracle.com> wrote:
> All changes (hotspot and top-level) are now in the jdk9/hs-rt forest.
>
> David
>
>
> On 14/07/2014 9:09 PM, David Holmes wrote:
>>
>> On 14/07/2014 7:44 PM, Volker Simonis wrote:
>>>
>>> On Sun, Jul 13, 2014 at 11:22 PM, David Holmes
>>> <david.holmes@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/
>>
>>
>> Yes good point. ia64 should be eradicated from the build system :)
>>
>> I will put this altogether in the AM.
>>
>>> I've also added Vladimir as reviewer.
>>
>>
>> Great
>>
>> Thanks,
>> David
>>
>>
>>> 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@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@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
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic