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

[David Holmes]

On Wed, 9 Mar 2022 10:11:06 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> @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

@tstuefe  do you have some examples of (3)? I don't like introducing a fake, seemingly general-purpose, API for something that is very much platform specific. I do dislike intensely the way the ThreadWX changes pollute shared code, and as has been said in other reviews of that code, there really should be a much cleaner/clearer place where these transitions occur - if we can find it.

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

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