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

[Thomas Stuefe]

On Wed, 9 Mar 2022 07:30:43 GMT, David Holmes <dholmes at 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