On Mon, 7 Mar 2022 11:29:08 GMT, Johannes Bechberger <[email protected]>
wrote:
> The WXMode for the current thread (on MacOS aarch64) is currently stored in
> the thread class which is unnecessary as the WXMode is bound to the current
> OS thread, not the current instance of the thread class.
> This pull request moves the storage of the current WXMode into a thread local
> global variable in `os` and changes all related code. SafeFetch depended on
> the existence of a thread object only because of the WXMode. This pull
> request therefore removes the dependency, making SafeFetch usable in more
> contexts.
Hi Johannes, just some drive-by comments, not a full review. Also please see my
comment toward David, proposing a more generic interface in os instead.
Cheers, Thomas
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 537:
> 535: #endif
> 536:
> 537: static THREAD_LOCAL WXMode _wx_state = WXUnknown;
All this wx coding inside bsd sources should be guarded with `__APPLE__` out of
politeness toward the BSDs.
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 552:
> 550: _wx_state = new_state;
> 551: pthread_jit_write_protect_np(_wx_state == WXExec);
> 552: }
I would simplify this:
if (_wx_state == unknown) {
_wx_state = write; // No way to know but we assume the original state is
"writable, not executable"
}
WXMode old = _wx_state;
_wx_state = new_state;
pthread_jit_write_protect_np(_wx_state == WXExec);
}
that is simpler and avoids calling pthread_jit_write_protect_np twice for the
"unknown->exec" transition.
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 558:
> 556: void os::current_thread_reset_wx() {
> 557: current_thread_change_wx(WXWrite);
> 558: }
I find the naming a bit misleading. You use this as initialization, so I would
call it "init" something.
Then, I'm not sure it is even needed. I know you just transformed it from the
original `init_wx()`, so the question is directed more at the original authors
(@AntonKozlov?).
AFAIU we use this to initialize wxstate for newly attached threads to "dont
execute". But should this not already be the case? And if its not - e.g.
because that thread had been calling into another library that also does call
generated code - is it not impolite to switch the state to "executable false"?
I know this is highly unlikely, I just try to understand.
src/hotspot/share/runtime/os.hpp line 943:
> 941: static WXMode current_thread_change_wx(WXMode new_state);
> 942:
> 943: static void current_thread_reset_wx();
Please add comments what this is supposed to do
-------------
Changes requested by stuefe (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7727
