On 14/07/2014 7:44 PM, Volker Simonis wrote:
On Sun, Jul 13, 2014 at 11:22 PM, David Holmes <[email protected]> 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 <[email protected]>
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 <[email protected]>
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
