RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Stefan Karlsson]

On Mon, 25 Jan 2021 19:38:16 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 two additional commits since the last revision:
> 
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin

>     * 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.

I wonder if this is the right choice. I think it would have been good if this could have been completely hidden in the transition classes. However, that doesn't seem to have been enough and now there are explicit Thread::WXWriteFromExecSetter instances where it's not at all obvious why they are needed. For example, why was it added here?:
OopStorageParIterPerf::~OopStorageParIterPerf() {
  // missing transition to vm state
  Thread::WXWriteFromExecSetter wx_write;
  _storage.release(_entries, ARRAY_SIZE(_entries));
}
You need to dig down in the OopStorage implementation to find that it's because it uses SafeFetch, which has the opposite transition:
inline int SafeFetch32(int* adr, int errValue) {
  assert(StubRoutines::SafeFetch32_stub(), "stub not yet generated");
  Thread::WXExecFromWriteSetter wx_exec;
  return StubRoutines::SafeFetch32_stub()(adr, errValue);
}
I think this adds complexity to code that shouldn't have to deal with this. I'm fine with having to force devs / code that writes to exec memory to have to deal with a bit more complexity, but it seems wrong to let this leak out to code that is staying far away from that.

For my understanding, was this choice made because the nmethods instances (and maybe other types as well) are placed in the code cache memory segment, which is executable? If the code cache only contained the JIT:ed code, and not object instances, I think this could more easily have been contained a bit more.

If the proposed solution is going to stay, maybe this could be made a little bit nicer by changing the SafeFetch implementation to use a new scoped object that doesn't enforce the "from" state to be "write"? Instead it could check if the state is "write" and only then flip the state back and forth. I think, this would remove the change needed OopStorageParIterPerf, and probably other places as well.

src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp line 38:

> 36: #include "runtime/os.hpp"
> 37: #include "runtime/stubRoutines.hpp"
> 38: #include "runtime/stubRoutines.inline.hpp"

Remove stubRutines.hpp line

src/hotspot/share/runtime/stubRoutines.inline.hpp line 29:

> 27: 
> 28: #include <runtime/thread.hpp>
> 29: #include <runtime/stubRoutines.hpp>

Replace < > with " "

src/hotspot/os/aix/os_aix.cpp line 70:

> 68: #include "runtime/statSampler.hpp"
> 69: #include "runtime/stubRoutines.hpp"
> 70: #include "runtime/stubRoutines.inline.hpp"

Remove stubRutines.hpp line

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

Changes requested by stefank (Reviewer).

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