On Fri, 22 Jan 2021 18:49:42 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)
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 86:
> 84:
> 85: switch (_num_int_args) {
> 86: case 0:
I don't think you need such a large switch statement. I think this can be
expressed as
if (num_int_args <= 6) {
ldr(as_Register(num_int_args + 1), src);
... etc.
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 43:
> 41: //describe amount of space in bytes occupied by type on native stack
> 42: #ifdef __APPLE__
> 43: const int nativeByteSpace = sizeof(jbyte);
I'm not sure these names should be in the global name space, and they're not
very descriptive. Something like NativeStack::byteSpace would be better. Then
you can use them with a `using namespace NativeStack` declaration.
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 433:
> 431: store_and_inc(_to, from_obj, nativeShortSpace);
> 432:
> 433: _num_int_args++;
Please lift this increment out of the conditional.
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 394:
> 392:
> 393: class SlowSignatureHandler
> 394: : public NativeSignatureIterator {
SlowSignatureHandler is turning into a maintenance nightmare. This isn't the
fault of this commit so much as some nasty cut-and=paste programming in the
code you're editing. It might well be better to rewrite this whole thing to use
a table-driven approach, with a table that contains the increment and the size
of each native type: then we'd have only a single routine which could pass data
of any type, and each OS needs only supply a table of constants.
src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5272:
> 5270: void MacroAssembler::get_thread(Register dst) {
> 5271: RegSet saved_regs = RegSet::range(r0, r1) +
> BSD_ONLY(RegSet::range(r2, r17)) + lr - dst;
> 5272: push(saved_regs, sp);
This isn't very nice.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2200
