RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
Daniel D.Daugherty
dcubed at openjdk.java.net
Tue Feb 2 23:13:00 UTC 2021
On Tue, 2 Feb 2021 11:59:08 GMT, Anton Kozlov <akozlov at 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 one additional commit since the last revision:
>
> support macos_aarch64 in hsdis
For platform files that were copied from other ports to this port, if the file wasn't
changed I presume the copyright years are left alone. If the file required changes
for this port, I expect the year to be updated to 2021. How are you verifying that
these copyright years are being properly managed on the new files?
For the new W^X helpers, e.g., WXWriteFromExecSetter, a short comment
explaining why one was landed in a particular place would help reviewers.
Also see my comment about creating a new ThreadToNativeWithWXExecFromVM
helper.
I'm stopping my review with all the src/hotspot files done for now.
make/autoconf/flags.m4 line 140:
> 138: else
> 139: MACOSX_VERSION_MIN=10.12.0
> 140: fi
Not something that needs to be addressed here, but these changes
illustrate that our collective use of macOSX/MACOSX/MacOSX names
are tied to the fact that the macOS major version number was at 10
for a very long time.
@magicus - Do we have an RFE to rename MACOSX or are we sticking
with it and evolving our interpretation of the 'X' from '10' to */splat/asterik?
make/common/NativeCompilation.gmk line 1178:
> 1176: endif
> 1177: # This only works if the openjdk_codesign identity is present on the system. Let
> 1178: # silently fail otherwise.
Might want to add a comment here:
# The '-f' option will replace an existing signature if one exists.
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 41:
> 39: #define __ _masm->
> 40:
> 41: //describe amount of space in bytes occupied by type on native stack
nit - needs a space after `//`
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 83:
> 81: // On macos/aarch64 native stack is packed, int/float are using only 4 bytes
> 82: // on stack. Natural alignment for types are still in place,
> 83: // for example double/long should be 8 bytes alligned
nit typo: s/alligned/aligned/
And sentence should end with a period.
src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 323:
> 321: str(zr, Address(rthread, JavaThread::last_Java_pc_offset()));
> 322:
> 323: str(zr, Address(rthread, JavaFrameAnchor::saved_fp_address_offset()));
I don't think this switch from `JavaThread::saved_fp_address_offset()`
to `JavaFrameAnchor::saved_fp_address_offset()` is correct since
`rthread` is still used and is a JavaThread*. The new code will give you:
`rthread` + offset of the `saved_fp_address` field in a JavaFrameAnchor
The old code gave you:
`rthread` + offset of the `saved_fp_address` field in the JavaFrameAnchor field in the JavaThread
Those are not the same things.
src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp line 31:
> 29: #include "asm/assembler.inline.hpp"
> 30: #include "oops/compressedOops.hpp"
> 31: #include "runtime/vm_version.hpp"
It's not clear why this include needed to be added.
src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 810:
> 808: #ifdef __APPLE__
> 809: // Less-than word types are stored one after another.
> 810: // The code unable to handle this, bailout.
Perhaps: // The code is unable to handle this so bailout.
src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 837:
> 835: #ifdef __APPLE__
> 836: // Less-than word types are stored one after another.
> 837: // The code unable to handle this, bailout.
Perhaps: // The code is unable to handle this so bailout.
src/hotspot/os/aix/os_aix.cpp line 69:
> 67: #include "runtime/sharedRuntime.hpp"
> 68: #include "runtime/statSampler.hpp"
> 69: #include "runtime/stubRoutines.inline.hpp"
It is not clear why this include to inline-include change was made.
src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp line 37:
> 35: #include "runtime/init.hpp"
> 36: #include "runtime/os.hpp"
> 37: #include "runtime/stubRoutines.inline.hpp"
It is not clear why this include to inline-include change was made.
src/hotspot/os/windows/os_windows.cpp line 65:
> 63: #include "runtime/sharedRuntime.hpp"
> 64: #include "runtime/statSampler.hpp"
> 65: #include "runtime/stubRoutines.inline.hpp"
It is not clear why this include to inline-include change was made.
src/hotspot/os_cpu/bsd_aarch64/atomic_bsd_aarch64.hpp line 59:
> 57: }
> 58:
> 59: // __attribute__((unused)) on dest is to get rid of spurious GCC warnings.
Is the GCC comment appropriate? Does this file get built
with GCC or just Apple compilers?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 92:
> 90: # define DEFAULT_MAIN_THREAD_STACK_PAGES 2048
> 91: # define OS_X_10_9_0_KERNEL_MAJOR_VERSION 13
> 92: #endif
Does this work around for Maveriks (10.9.0) need to be here?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 103:
> 101: // 10.5 UNIX03 member name prefixes
> 102: #define DU3_PREFIX(s, m) __ ## s.__ ## m
> 103: # else
Does this work around for macOS 10.5 need to be here?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 195:
> 193: frame os::get_sender_for_C_frame(frame* fr) {
> 194: return frame(fr->link(), fr->link(), fr->sender_pc());
> 195: }
Is this file going to be built by GCC or just macOS compilers?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 221:
> 219: assert(sig == info->si_signo, "bad siginfo");
> 220: }
> 221: */
Should this code be deleted? From here and from where it was copied
from if it is also commented out there...
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 363:
> 361: address pc = os::Posix::ucontext_get_pc(uc);
> 362:
> 363: if (pc != addr && uc->context_esr == 0x9200004F) { //TODO: figure out what this value means
Is this TODO going to be resolved by this port?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 374:
> 372:
> 373: last_addr = (address) -1;
> 374: } else if (pc == addr && uc->context_esr == 0x8200000f) { //TODO: figure out what this value means
Is this TODO going to be resolved by this port?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 435:
> 433: // | |\ Java thread created by VM does not have glibc
> 434: // | glibc guard page | - guard, attached Java thread usually has
> 435: // | |/ 1 glibc guard page.
Is this code going to be built by GCC (with glibc) or will only
macOS compilers and libraries be used?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 486:
> 484: }
> 485: }
> 486: }
This appears to be a mix for Mavericks (10.9) and 10.12
work arounds. Is this code needed by this project?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.hpp line 45:
> 43: // Atomically copy 64 bits of data
> 44: static void atomic_copy64(const volatile void *src, volatile void *dst) {
> 45: *(jlong *) dst = *(const jlong *) src;
Is this construct actually atomic on aarch64?
src/hotspot/os_cpu/bsd_aarch64/thread_bsd_aarch64.cpp line 43:
> 41: assert(Thread::current() == this, "caller must be current thread");
> 42: return pd_get_top_frame(fr_addr, ucontext, isInJava);
> 43: }
Is AsyncGetCallTrace() being supported by this port?
src/hotspot/os_cpu/aix_ppc/os_aix_ppc.hpp line 43:
> 41:
> 42: private:
> 43:
'private' usually has once space in front of it.
Also why the blank line after it?
src/hotspot/os_cpu/aix_ppc/os_aix_ppc.hpp line 46:
> 44: static void current_thread_enable_wx_impl(WXMode mode) { }
> 45:
> 46: public:
'public' usually has one space in front of it.
src/hotspot/os_cpu/bsd_x86/os_bsd_x86.hpp line 41:
> 39:
> 40: private:
> 41:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/bsd_x86/os_bsd_x86.hpp line 44:
> 42: static void current_thread_enable_wx_impl(WXMode mode) { }
> 43:
> 44: public:
'public' usually has one space in front of it.
src/hotspot/os_cpu/bsd_zero/os_bsd_zero.hpp line 57:
> 55:
> 56: private:
> 57:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/bsd_zero/os_bsd_zero.hpp line 60:
> 58: static void current_thread_enable_wx_impl(WXMode mode) { }
> 59:
> 60: public:
'public' usually has one space in front of it.
src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.hpp line 43:
> 41:
> 42: private:
> 43:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.hpp line 46:
> 44: static void current_thread_enable_wx_impl(WXMode mode) { }
> 45:
> 46: public:
'public' usually has one space in front of it.
src/hotspot/os_cpu/linux_arm/os_linux_arm.hpp line 74:
> 72:
> 73: private:
> 74:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/linux_arm/os_linux_arm.hpp line 77:
> 75: static void current_thread_enable_wx_impl(WXMode mode) { }
> 76:
> 77: public:
'public' usually has one space in front of it.
src/hotspot/os_cpu/linux_ppc/os_linux_ppc.hpp line 36:
> 34:
> 35: private:
> 36:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/linux_ppc/os_linux_ppc.hpp line 39:
> 37: static void current_thread_enable_wx_impl(WXMode mode) { }
> 38:
> 39: public:
'public' usually has one space in front of it.
src/hotspot/os_cpu/linux_s390/os_linux_s390.hpp line 35:
> 33:
> 34: private:
> 35:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/linux_s390/os_linux_s390.hpp line 38:
> 36: static void current_thread_enable_wx_impl(WXMode mode) { }
> 37:
> 38: public:
'public' usually has one space in front of it.
src/hotspot/os_cpu/linux_x86/os_linux_x86.hpp line 54:
> 52:
> 53: private:
> 54:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/linux_x86/os_linux_x86.hpp line 57:
> 55: static void current_thread_enable_wx_impl(WXMode mode) { }
> 56:
> 57: public:
'public' usually has one space in front of it.
src/hotspot/os_cpu/linux_zero/os_linux_zero.hpp line 94:
> 92:
> 93: private:
> 94:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/linux_zero/os_linux_zero.hpp line 97:
> 95: static void current_thread_enable_wx_impl(WXMode mode) { }
> 96:
> 97: private:
I think this should 'public' (with one space in front of it).
src/hotspot/os_cpu/windows_aarch64/os_windows_aarch64.hpp line 37:
> 35:
> 36: private:
> 37:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/windows_aarch64/os_windows_aarch64.hpp line 40:
> 38: static void current_thread_enable_wx_impl(WXMode mode) { }
> 39:
> 40: public:
'public' usually has one space in front of it.
src/hotspot/os_cpu/windows_x86/os_windows_x86.hpp line 47:
> 45:
> 46: private:
> 47:
'private' usually has one space in front of it.
Also, why the blank line after it?
src/hotspot/os_cpu/windows_x86/os_windows_x86.hpp line 50:
> 48: static void current_thread_enable_wx_impl(WXMode mode) { }
> 49:
> 50: public:
'public' usually has one space in front of it.
src/hotspot/share/gc/shared/oopStorage.cpp line 40:
> 38: #include "runtime/os.hpp"
> 39: #include "runtime/safepoint.hpp"
> 40: #include "runtime/stubRoutines.inline.hpp"
The reason for switching from include to inline-include is not clear.
src/hotspot/share/logging/logStream.hpp line 35:
> 33: class LogStream : public outputStream {
> 34: // see test/hotspot/gtest/logging/test_logStream.cpp
> 35: friend class LogStreamTest;
It's not clear why this change is made for this port.
src/hotspot/share/prims/jni.cpp line 3930:
> 3928:
> 3929: // Go to the execute mode, the initial state of the thread on creation.
> 3930: // Use os interface as the thread is not a java one anymore.
Perhaps: s/not a java one anymore./not a JavaThread anymore./
src/hotspot/share/prims/nativeEntryPoint.cpp line 45:
> 43: guarantee(status == JNI_OK && !env->ExceptionOccurred(),
> 44: "register jdk.internal.invoke.NativeEntryPoint natives");
> 45: JNI_END
I thought that jcheck caught a missing new-line?
src/hotspot/share/runtime/globals.hpp line 1855:
> 1853: \
> 1854: product(intx, UnguardOnExecutionViolation, 0, \
> 1855: "Unguard page and retry on no-execute fault " \
Taking away the "(Win32 only)" and not replacing it with new text feels wrong.
I can't imagine that this flag works on any additional platforms except for
macos-aarch64.
src/hotspot/share/runtime/os.cpp line 58:
> 56: #include "runtime/osThread.hpp"
> 57: #include "runtime/sharedRuntime.hpp"
> 58: #include "runtime/stubRoutines.inline.hpp"
The reason for switching from include to inline include is not clear.
src/hotspot/share/runtime/objectMonitor.cpp line 52:
> 50: #include "runtime/safepointMechanism.inline.hpp"
> 51: #include "runtime/sharedRuntime.hpp"
> 52: #include "runtime/stubRoutines.inline.hpp"
The reason for switching from include to inline include is not clear.
src/hotspot/share/runtime/stubRoutines.cpp line 34:
> 32: #include "runtime/timerTrace.hpp"
> 33: #include "runtime/sharedRuntime.hpp"
> 34: #include "runtime/stubRoutines.inline.hpp"
The reason for switching from include to inline include is not clear.
src/hotspot/share/runtime/stubRoutines.inline.hpp line 1:
> 1: /*
NOW I understand the reason for switching from include to inline-include.
Is there a reason that this change is part of this project and not extracted
into a separate RFE. That would reduce the number of files touched by
this project.
src/hotspot/share/runtime/thread.cpp line 3991:
> 3989: JavaThread* thread = JavaThread::current();
> 3990: ThreadToNativeFromVM ttn(thread);
> 3991: Thread::WXExecFromWriteSetter wx_exec;
I saw somewhere in this review a comment about why this new
WXExecFromWriteSetter helper isn't folded into ThreadToNativeFromVM
and I understand that not every current ThreadToNativeFromVM needs
the new helper. If the vast majority of the ThreadToNativeFromVM
locations need WXExecFromWriteSetter, then perhaps those locations
that currently have a ThreadToNativeFromVM followed by the new
WXExecFromWriteSetter should use a new helper:
ThreadToNativeWithWXExecFromVM
so we'll see changes from:
ThreadToNativeFromVM -> ThreadToNativeWithWXExecFromVM
where we need them and hopefully a short comment can be added
at the same time to explain the need for WXExec. This will allow us
to easily distinguish ThreadToNativeFromVM locations that DO NOT
need WXExec from those that DO need it.
src/java.base/macosx/native/libjli/java_md_macosx.m line 210:
> 208: if (preferredJVM == NULL) {
> 209: #if defined(__i386__)
> 210: preferredJVM = "client";
#if defined(__i386__)
preferredJVM = "client";
Not your bug, but Oracle/OpenJDK never supported 32-bit __i386__ on macOS.
-------------
Changes requested by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2200
More information about the security-dev
mailing list