On Tue, 8 Mar 2022 00:25:51 GMT, Johannes Bechberger <[email protected]>
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
