On Sun, 24 Jan 2021 15:32:59 GMT, Anton Kozlov <akoz...@openjdk.org> wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Address feedback for signature generators
>  - Enable -Wformat-nonliteral back

Changes requested by ihse (Reviewer).

make/autoconf/jvm-features.m4 line 269:

> 267:     if test "x$OPENJDK_TARGET_OS" != xaix && \
> 268:         !( test "x$OPENJDK_TARGET_OS" = "xmacosx" && \
> 269:         test "x$OPENJDK_TARGET_CPU" = "xaarch64" ) ; then

This test is making my head spin. Can't you just invert it to this structure:

if OS=aix || OS+CPU = mac+aarch64; then
  no
else
 yes
fi

make/autoconf/platform.m4 line 75:

> 73:         ;;
> 74:       esac
> 75:       ;;

This is solved in the wrong place. This code should just use the result from 
`config.guess/config.sub`. These files are imported from the autoconf project. 
Unfortunately, they have changed the license to one incompatible with OpenJDK, 
so we are stuck with an old version. Instead, we have created a "bugfix 
wrapper" that calls the original `autoconf-config.guess/sub` and fixes up the 
result, with stuff like this.

make/autoconf/platform.m4 line 273:

> 271:   # Convert the autoconf OS/CPU value to our own data, into the 
> VAR_OS/CPU/LIBC variables.
> 272:   PLATFORM_EXTRACT_VARS_FROM_OS($build_os)
> 273:   PLATFORM_EXTRACT_VARS_FROM_CPU($build_cpu, $build_os)

Fixing the comment above means this change, and the one below, can be reverted.

make/common/NativeCompilation.gmk line 1180:

> 1178:                 # silently fail otherwise.
> 1179:                 ifneq ($(CODESIGN), )
> 1180:                   $(CODESIGN) -f -s "$(MACOSX_CODESIGN_IDENTITY)" 
> --timestamp --options runtime \

What does -f do, and is it needed for macos-x64 as well?

make/modules/jdk.hotspot.agent/Lib.gmk line 34:

> 32: 
> 33: else ifeq ($(call isTargetOs, macosx), true)
> 34:   SA_CFLAGS := -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \

Is this really proper for macos-x64? I thought we used -Damd64 as a marker for 
all macos-x64 C/C++ files. (Most files get their flags from the common setup in 
configure, but SA is a different beast due to historical reasons).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2200

Reply via email to