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