On Mon, 25 Jan 2021 19:38:16 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:
>
> - 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
