On Wed, 9 Mar 2022 19:03:16 GMT, Anton Kozlov <akoz...@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
