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

[Thomas Stuefe]

On Wed, 9 Mar 2022 19:03:16 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   current_thread_wx -> ThreadWX
>
> https://github.com/openjdk/jdk/compare/master...478ec1a7ca2c72e5916b28613a4875aa2ee1a793 touches more places than a targeted change in ThreadWXEnable... I'm not sure the real nesting is required for a thread that is not registered properly with VM. The initial state is always assumed for the NULL Thread. The SafeFetch assembly does not do up-calls to VM. I don't see why we'd need runtime tracking of WX state. The state is either WXExec for SafeFetch assembly, or unknown -- which we assume to be WXWrite regardless of approach taken.
> 
> Nesting was implemented to reduce the amount of changes in JVM (yes, WX code scattered around the VM less than it could be :)), but it is possible to avoid runtime WX tracking if you always know the state, like we do if Thread == NULL.

Hi @AntonKozlov,

> [master...478ec1a](https://github.com/openjdk/jdk/compare/master...478ec1a7ca2c72e5916b28613a4875aa2ee1a793) touches more places than a targeted change in ThreadWXEnable... I'm not sure the real nesting is required for a thread that is not registered properly with VM.

Arguably we should restore, upon leaving the VM, the state that has been present before. Because a native thread may already have modified the wx state and overruling it is just rude. But I offhand don't see a way to do this since we have no way (?) to query the current state.



> The change proposes to assume WXWrite as the initial state. Have you considered to extend ThreadWXEnable to fix the assert failure? Something like below (I have not tried to compile though). The refactoring looks OK, but it makes sense to separate it from functional change.
> 
> ```
> class ThreadWXEnable  {
>   Thread* _thread;
>   WXMode _old_mode;
> 
> public:
>   ThreadWXEnable(WXMode new_mode, Thread* thread) :
>     _thread(thread)
>   {
>     if (_thread) {
>       _old_mode = _thread->enable_wx(new_mode);
>     } else {
>       os::current_thread_enable_wx(new_mode);
>       _old_mode = WXWrite;
>     }
>   }
>   ~ThreadWXEnable() {
>     if (_thread) {
>       _thread->enable_wx(_old_mode);
>     } else {
>       os::current_thread_enable_wx(_old_mode);
>     }
>   }
> };
> ```

I honestly don't find this easier than the solution @parttimenerd proposes, using a OS thread local. Using an OS thread local makes this whole system independent from Thread, so you don't need to know about Thread and don't rely on Thread::current being present.

It also would be slightly faster. Using Thread, we'd access TLS to get Thread::current, then dereference that to read the wx state . OTOH using OS TLS, we access TLS to get the wx state directly. We save one dereference.

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

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