On Wed, 9 Mar 2022 07:30:43 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> I don't know why the Linux x86 build fails.
>>
>> I tested the current version with code related to #7591 and it seems to fix
>> the remaining problems (I tested it also with NMT enabled).
>
> @parttimenerd please never force-push in an active review as it completely
> destroys the review history and comment context!
Hi @dholmes-ora , @parttimenerd
I'd like to argue again for my proposal from before.
All this is contrary to how we deal with platform dependencies normally.
Normally, we
1) either keep the whole code in platform specific places - including callers.
If that is possible.
2) If there are very few callers in shared places, we keep implementation in
platform branch and #ifdef the callsites in shared code out to the affected
platforms
3) If there are many callers in shared code or if it looks like it may be
useful on other platforms too at some point, we usually wrap the logic in a
platform generic function behind a descriptive name, which we stub out for the
unaffected platforms.
(3) is very common.
My proposal from before would make it possible to really hide all WX logic from
shared code:
shared/runtime/os.hpp
```
class os {
...
// Platform specific hook to prepare the current thread for calling generated
code
void enable_jit_calls_for_current_thread() NOT_MACOS_AARCH64({})
// Platform specific hook to clean up the current thread after calling into
generated code
void disable_jit_calls_for_current_thread() NOT_MACOS_AARCH64({})
class ThreadEnableJitCallsMark: public StackObj {
public:
ThreadEnableJitCallsMark() { enable_jit_calls_for_current_thread(); }
~ThreadEnableJitCallsMark() { disable_jit_calls_for_current_thread(); }
}
(ThreadEnableJitCallsMark could be optionally spread out into separate include)
os.bsd_aarch64.cpp
void os::enable_jit_calls_for_current_thread() {
... blabla ... pthread_jit_write_protect_np();
}
void os::disable_jit_calls_for_current_thread() {
... blabla ... pthread_jit_write_protect_np();
}
Thats very little code. It effectively hides all platform details where they
belong, away from shared code. In shared code, you use either one of
`os::(enable|disable)_jit_calls_for_current_thread()` or the companion
`os::ThreadEnableJitCallsMark`. Advantages would be:
- Call sites in shared code are now easier to mentally parse.
`os::disable_jit_calls_for_current_thread()` is much clearer than
`MACOS_AARCH64_ONLY(os::ThreadWX::Enable __wx(os::ThreadWX::Write));`.
- We don't need MAC_AARCH64_ONLY in shared code
- We don't need the enums in shared code. Dont need to understand what they do,
nor the difference between "Write" and "Exec".
Side note, I'd also suggest a different name for the RAII object to something
like "xxxMark". That is more according to hotspot customs.
----
Cheers, Thomas
-------------
PR: https://git.openjdk.java.net/jdk/pull/7727
