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

[David Holmes]

On Tue, 8 Mar 2022 00:25:51 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.
>
> Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Move WX functionality into os specific files

This is looking good. A few additional comments below.

Thanks,
David

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 538:

> 536: 
> 537: #ifdef __APPLE__
> 538: static THREAD_LOCAL os::WXMode _wx_state = os::WXUnknown;

Please add a blank line between the THREAD_LOCAL and the next method. Or even move this THREAD_LOCAL to just before `os::current_thread_change_wx`.

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 547:

> 545:   if (_wx_state == WXUnknown) {
> 546:     _wx_state = os::WXWrite; // No way to know but we assume the original state is "writable, not executable"
> 547:   }

Given this can't you just initialize to WXWrite and do away with WXUnknown?

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.hpp line 57:

> 55:   static void current_thread_init_wx();
> 56: 
> 57:   static void current_thread_assert_wx_state(WXMode expected);

Can we move all these into the ThreadWXEnable class so they are not in the os namespace? Even the enum could move - though it will make the use-sites a bit more verbose. I won't insist on pushing this WX stuff even deeper, but if anyone else thinks it is a good idea ... :)

src/hotspot/share/prims/jni.cpp line 97:

> 95: #include "utilities/macros.hpp"
> 96: #include "utilities/vmError.hpp"
> 97: #include "runtime/thread.inline.hpp"

Why do we need this? Why do we not include os.hpp?

src/hotspot/share/runtime/safefetch.inline.hpp line 31:

> 29: 
> 30: #include "runtime/stubRoutines.hpp"
> 31: #include "runtime/os.hpp"

Please list the includes alphabetically.

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

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