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 David,

> The general idea seems good (pity it touches so many files, but then I've never liked any of this WX support precisely because it is so invasive of shared code). I agree that safeFetch should not have become dependent on Thread::current existing, but I have to wonder whether we can just skip the WX code if there is no current thread? If the thread is not attached to the VM then what does it even mean to manipulate the WX state of an unknown thread?

We need to change the wx state of the current pthread in order to be able to execute stub routines. Otherwise, we would crash right away when trying to execute the SafeFetch stub.

And that is a valid requirement. Let's say we crash in a native thread, unrelated to and completely oblivious of the JVM it shares the process with. We'd still want to see e.g. native crash information, stack frames, maybe register region information etc - all that stuff that may require SafeFetch. In fact, this patch is related to Johannes other PR where he modified stack frame walking to check that the registers point into valid memory. 

> 
> That aside, with this change I think we can move the conditional WX code out of the shared os.hpp and bury it down in os_bsd_aarch64.hpp where it actually belongs.

Oh yes!

> 
> I'd even like to see threadWXSetters.inline.hpp moved to being in src/os_cpu/bsd_aarch64/ if feasible - I'm not sure what include would be needed for the callsites to function - os.hpp I presume?

I agree, all that wx stuff should be limited to os/bsd or os/bsd_aarch. 

We could have generic wrappers like:


class os {
...
// Platform does whatever needed to prepare for execution of generated code inside the current thread
os::pre_current_thread_jit_call() NOT_MACOS_AARCH64({})
// Platform does whatever needed to clean up after executing generated code inside the current thread
os::post_current_thread_jit_call() NOT_MACOS_AARCH64({})


(Macro does not yet exist, but MACOS_AARCH64_ONLY does)

--

Side note, I think we have reached a point where it would be cleaner to split xxxBSD and MacOS sources. E.g. this wx stuff should be limited to MacOS too, and we have more and more `__APPLE_` only sections.

Cheers, Thomas

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

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