On Tue, 9 Mar 2021 17:55:12 GMT, Anton Kozlov <[email protected]> wrote:
>> src/hotspot/share/runtime/thread.cpp line 2515:
>>
>>> 2513: void JavaThread::check_special_condition_for_native_trans(JavaThread
>>> *thread) {
>>> 2514: // Enable WXWrite: called directly from interpreter native wrapper.
>>> 2515: MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXWrite, thread));
>>
>> FWIW, I personally think that adding these MACOS_AARCH64_ONLY usages at the
>> call sites increase the line-noise in the affected functions. I think I
>> would have preferred a version:
>> ThreadWXEnable(WXMode new_mode, Thread* thread = NULL) {
>> MACOS_AARCH64_ONLY(initialize(new_mode, thread);) {}
>> void initialize(...); // Implementation in thread_bsd_aarch64.cpp (alt.
>> inline.hpp)
>> With that said, I'm fine with taking this discussion as a follow-up.
>
> The former version used no such macros. I like that now it's clear the W^X
> management is relevant to macos/aarch64 only. I see the point to move the
> pre-processor condition into the class implementation. But I think it will
> bring a bit of inconsistency, as the rest of W^X implementation is explicitly
> guarded by preprocessor conditionals. I've also tried to push macro
> conditionals as far as possible down to implementation, providing a kind of
> generalized W^X interface. That required a few artificial decisions, e.g. how
> would we call the mode we execute on the rest of platforms with write and
> execute allowed, WXWriteExec?.. I abandoned that attempt.
I think we would use the same names, but I haven't given it more thought. I
might take a look at this after this has been integrated.
>> src/hotspot/share/runtime/thread.hpp line 848:
>>
>>> 846: void init_wx();
>>> 847: WXMode enable_wx(WXMode new_state);
>>> 848: #endif // __APPLE__ && AARCH64
>>
>> Now that this is only compiled into macOS/AArch64, could this be moved over
>> to thread_bsd_aarch64.hpp? The same goes for the associated functions.
>
> The thread_bsd_aarch64.hpp describes a part of JavaThread, while this block
> belongs to Thread for now. Since W^X is an attribute of any operating system
> thread, I assumed Thread to be the right place for W^X bookkeeping.
>
> In most cases, we manage W^X state of JavaThread. But sometimes a GC thread
> needs the WXWrite state, or safefetch is called from non-JavaThread. Probably
> this can be dealt with (e.g. GCThread to always have the WXWrite state). But
> such change would be much more than a simple refactoring and it would require
> a significant amount of testing. Ideally, I would like to investigate this as
> a follow-up change, or at least after other fixes to this PR.
Good point about Thread vs JavaThread. Yes, this can be looked into as
follow-up cleanups.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2200
