RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port

[Andrew Haley]

On Fri, 22 Jan 2021 18:49:42 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)

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