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
