Thanks Magnus, see comments below ..
> On Dec 7, 2016, at 3:44 AM, Magnus Ihse Bursie
> <[email protected]> wrote:
>
> On 2016-12-05 16:23, Bob Vandette wrote:
>>> On Dec 2, 2016, at 8:04 PM, Vladimir Kozlov <[email protected]>
>>> <mailto:[email protected]> wrote:
>>>
>>> hi Bob,
>>>
>>> I would suggest to have separate webrevs for different repositories because
>>> different groups should look on them.
>> There are only 3 non hotspot files and they are on top. Forwarding to
>> build-dev for their review.
>
> Build changes mostly look good. A few comments:
>
> * We try to use the autoconf defined "$with_opt" variables only when checking
> and verifying arguments. In hotspot.m4, please let
> SETUP_HOTSPOT_TARGET_CPU_PORT assign the user specified value to e.g.
> OPENJDK_TARGET_CPU_PORT, and use that to do the check in
> HOTSPOT_SETUP_JVM_FEATURES.
I think it should be called HOTSPOT_TARGET_CPU_PORT since this variable only
impact which directory hotspot uses for it build.
I also removed a redundant if test "x$with_cpu_port" != x; in
SETUP_HOTSPOT_TARGET_CPU_PORT.
+ # Override hotspot cpu definitions for ARM platforms
+ if test "x$OPENJDK_TARGET_CPU" = xarm; then
+ HOTSPOT_TARGET_CPU=arm_32
+ HOTSPOT_TARGET_CPU_DEFINE="ARM32"
+ JVM_LDFLAGS="$JVM_LDFLAGS -fsigned-char"
+ JVM_CFLAGS="$JVM_CFLAGS -DARM -fsigned-char"
+ elif test "x$OPENJDK_TARGET_CPU" = xaarch64 && test
"x$HOTSPOT_TARGET_CPU_PORT" = xarm64; then
+ HOTSPOT_TARGET_CPU=arm_64
+ HOTSPOT_TARGET_CPU_ARCH=arm
+ JVM_LDFLAGS="$JVM_LDFLAGS -fsigned-char"
+ JVM_CFLAGS="$JVM_CFLAGS -DARM -fsigned-char"
+ fi
+
+AC_DEFUN([SETUP_HOTSPOT_TARGET_CPU_PORT],
+[
+ AC_ARG_WITH(cpu-port, [AS_HELP_STRING([--with-cpu-port],
+ [specify sources to use for Hotspot 64-bit ARM port (arm64,aarch64)
@<:@aarch64@:>@ ])])
+
+ if test "x$with_cpu_port" != x; then
+ if test "x$OPENJDK_TARGET_CPU" != xaarch64; then
+ AC_MSG_ERROR([--with-cpu-port only available on aarch64])
+ fi
+ if test "x$with_cpu_port" != xarm64 && \
+ test "x$with_cpu_port" != xaarch64; then
+ AC_MSG_ERROR([--with-cpu-port must specify arm64 or aarch64])
+ fi
+ HOTSPOT_TARGET_CPU_PORT = $with_cpu_port
+ fi
+])
+
+
>
> * In flags.m4, the SET_SHARED_LIBRARY_ORIGIN code checks
> OPENJDK_TARGET_CPU_ARCH. I'd prefer if it checked OPENJDK_TARGET_CPU instead,
> since explicitly checking the CPU_ARCH variable seems to indicate that we
> want to test more than a single CPU, but for arm there is only one. (At
> first, I thought this was a test for "arm64" as well. If this was the
> intention, then the code is broken.)
>
Ok.
> * In CompileJvm.gmk, you might want to rephrase the comment, since from now
> on both the original aarch64 and the new arm64 port are "open". What the code
> does is in fact to select between the two different aarch64 ports.
Done. I also found another comment that needed to be updated.
diff --git a/make/lib/CompileJvm.gmk b/make/lib/CompileJvm.gmk
--- a/make/lib/CompileJvm.gmk
+++ b/make/lib/CompileJvm.gmk
@@ -63,8 +63,8 @@
# INCLUDE_SUFFIX_* is only meant for including the proper
# platform files. Don't use it to guard code. Use the value of
# HOTSPOT_TARGET_CPU_DEFINE etc. instead.
-# Remaining TARGET_ARCH_* is needed to distinguish closed and open
-# 64-bit ARM ports (also called AARCH64).
+# Remaining TARGET_ARCH_* is needed to select the cpu specific
+# sources for 64-bit ARM ports (arm versus aarch64).
JVM_CFLAGS_TARGET_DEFINES += \
-DTARGET_ARCH_$(HOTSPOT_TARGET_CPU_ARCH) \
-DINCLUDE_SUFFIX_OS=_$(HOTSPOT_TARGET_OS) \
@@ -139,6 +139,20 @@
################################################################################
# Platform specific setup
+# ARM source selection
+
+ifeq ($(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU), linux-arm)
+ JVM_EXCLUDE_PATTERNS += arm_64
+
+else ifeq ($(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU), linux-aarch64)
+ # For 64-bit arm builds, we use the 64 bit hotspot/src/cpu/arm
+ # hotspot sources if HOTSPOT_TARGET_CPU_ARCH is set to arm.
+ # Exclude the aarch64 and 32 bit arm files for this build.
+ ifeq ($(HOTSPOT_TARGET_CPU_ARCH), arm)
+ JVM_EXCLUDE_PATTERNS += arm_32 aarch64
+ endif
+endif
+
Bob.
>
> /Magnus
>
>
>>
>>> For example, top repository and makefiles changes should be also reviewed
>>> on [email protected] <mailto:[email protected]>
>>>
>>> Why do you need cahnges in SA libproc.h?
>> The cross compilation toolchains we use do not define user_regs_struct or
>> user_pt_regs.
>>
>> I just looked again and there is a definition of struct user_regs in user.h.
>> I might be able to change the code to:
>>
>> #if defined(arm) || defined(arm64)
>> #define user_regs_struct user_regs
>> #endif
>>
>> This change would result in the exact same declaration based on the number
>> of registers
>> derived from the structure in user.h.
>>
>> Bob.
>>
>>
>>> I saw Hotspot changes before and I think they are fine (did not dive deep).
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 12/2/16 12:28 PM, Bob Vandette wrote:
>>>> Please review the proposed changes to be integrated under JEP 297.
>>>>
>>>> Summary:
>>>>
>>>> This JEP adds arm32 and arm64 Linux platform support to OpenJDK for JDK 9.
>>>>
>>>> This changeset also removes the support for the pregenerated interpreter
>>>> since
>>>> this is no longer supported.
>>>>
>>>> The addition of arm64 does not replace the existing aarch64 port. A new
>>>> configure
>>>> option (-with-cpu-port=) allows for the selection of the existing aarch64
>>>> versus the
>>>> 64-bit arm64 support being added via this JEP. Please refer to the JEP
>>>> for more details.
>>>>
>>>> JEP 297:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8168503
>>>> <https://bugs.openjdk.java.net/browse/JDK-8168503>
>>>>
>>>> Webrev:
>>>>
>>>> http://cr.openjdk.java.net/~bobv/8168503
>>>> <http://cr.openjdk.java.net/~bobv/8168503>
>>>>
>>>>
>>>> Note:
>>>>
>>>> A complete build-able forest containing these changes is located here:
>>>> http://hg.openjdk.java.net/aarch32-port/jdk9-arm3264
>>>> <http://hg.openjdk.java.net/aarch32-port/jdk9-arm3264>
>>>>
>>>> Thanks,
>>>> Bob Vandette
>>>>
>
