[OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v12]

[Stefan Karlsson]

On Tue, 9 Mar 2021 17:55:12 GMT, Anton Kozlov <akozlov at openjdk.org> 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