On Thu, 23 Mar 2023 07:27:14 GMT, Johannes Bechberger <jbechber...@openjdk.org> wrote:
> > You now effectively disable execution of generated code for the whole > > extend of AGCT? > > > > That's exactly what async-profiler does already > https://github.com/async-profiler/async-profiler/blob/c8de91df6b090af82e91a066deb81a3afb505331/src/profiler.cpp#L383, > I wonder why we don't have problems there with SafeFetch on older JVMs. > > > > > Okay. Though the prospect of async profiler modifying the code cache by > > walking the stack seems scary. > > > > We discussed it before. It's probably safe, but yes, my initial reaction was > also "why not just remove this", but it should have a performance impact. > > > > > Drive by comment: how async safe is WX enabler? If a thread is in the > > middle of it and we shoot a signal and enable, what will happen? > > > > This is a really good point. I think that making the field that stores this > information `volatile` could alleviate the problem (?). This would ensure > that no reordering takes place in: > > > > ``` > > _wx_state = WXWrite; > > os::current_thread_enable_wx(_wx_state); > > ``` > > (https://github.com/openjdk/jdk/blob/77cd917a97b184871ab2d3325ceb6c53afeca28b/src/hotspot/share/runtime/thread.inline.hpp#L78) If you look at the enable_wx function, there is a lack of atomicity of the operation. It checks the current state and only conditionally decides to change the state. And when it does there is a write of the new state and a call to actually flip the mode. Seems to me that there could be many problematic interleavings where the signal hits in this code, which could mess things up. This code was not designed to be async safe, and I'm not convinced that sprinkling in volatile really solves that problem. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13144#issuecomment-1480769232
