RFR: 8282475: SafeFetch should not rely on existence of Thread::current

[Thomas Stuefe]

On Mon, 7 Mar 2022 11:29:08 GMT, Johannes Bechberger <duke at 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