[aarch64-port-dev ] RFR: 8168503 JEP 297: Unified arm32/arm64 Port

Thu Dec 8 10:32:28 UTC 2016

On 2016-12-07 19:51, Bob Vandette wrote:
> Thanks Magnus,  see comments below ..
Your fixes look good. I'm ok with the build part of the patch now.

/Magnus
>
>> On Dec 7, 2016, at 3:44 AM, Magnus Ihse Bursie 
>> <magnus.ihse.bursie at oracle.com 
>> <mailto:magnus.ihse.bursie at oracle.com>> wrote:
>>
>> On 2016-12-05 16:23, Bob Vandette wrote:
>>>> On Dec 2, 2016, at 8:04 PM, Vladimir Kozlov<vladimir.kozlov at oracle.com>  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 onbuild-dev at openjdk.java.net
>>>>
>>>> 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
>>>>>
>>>>> Webrev:
>>>>>
>>>>> 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
>>>>>
>>>>> Thanks,
>>>>> Bob Vandette
>>>>>
>>
>