On Mon, 7 Mar 2022 11:29:08 GMT, Johannes Bechberger <d...@openjdk.java.net> 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