On Thu, 30 Oct 2025 22:47:41 GMT, Jorn Vernee <[email protected]> wrote:
>> See the JBS issue for a problem description. >> >> This patch changes the shared scope closure handshake to be able to handle >> 'arbitrary' Java frames on the stack during a scoped memory access. >> >> For the purposes of this change, we assume that 'arbitrary' is limited to >> the following: >> 1. Frames added by calling the constructor of `InternalError` as a result of >> a faulting access to a truncated memory-mapped file (see >> `HandshakeState::handle_unsafe_access_error`). This is the only handshake >> operation (i.e. may be triggered during a scoped access) that calls back >> into Java. >> 2. Frames added by a JVMTI agent that calls back into Java code while >> handling a JVMTI event that happens during a scoped access. >> 3. Any other Java code that runs as part of the linking process. >> >> For (1), we set a flag while we are create the `InternalError`. If a thread >> has that flag set, we know it is in the process of crashing already, so we >> don't have to inspect the stack at all. For (2), all bets are off, so we >> have to walk the entire stack. For (3), this patch switches the hard limit >> of 10 frames for the stack walk to instead bail out at the first frame >> outside of the `java.base` module. In most cases this speeds up the stack >> walk as well, if threads are running other code. >> >> The test `TestSharedCloseJFR` is added for scenario (1), and the test >> `TestSharedCloseJvmti` is added for scenario (2). Existing tests already >> cover scenario (3). >> >> Testing: tier 1-4 > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Update bug number in tests > > Co-authored-by: Chen Liang <[email protected]> Fix looks good to me. Only have some comments about the JVMTI agent case, thanks. src/hotspot/share/prims/scopedMemoryAccess.cpp line 53: > 51: #endif > 52: > 53: bool agents_loaded = JvmtiAgentList::has_agents(); I see that for dynamically loaded agents we add to the list after loading the agent. Maybe we should check `JvmtiEnvBase::environments_might_exist()`? src/hotspot/share/prims/scopedMemoryAccess.cpp line 94: > 92: > 93: static bool is_accessing_session(JavaThread* jt, oop session, bool& > in_scoped) { > 94: if (jt->is_throwing_unsafe_access_error()) { If we assume arbitrary Java code in JVMTI callbacks this might return true but the target could be in a different nested scoped access. I think we should check we are in the no agent case before bailing out. src/hotspot/share/runtime/javaThread.hpp line 1364: > 1362: JavaThread* _thread; > 1363: public: > 1364: ThrowingUnsafeAccessError(JavaThread* thread) : _thread(thread) { If we assume arbitrary Java code in JVMTI callbacks this could be executed recursively and `_throwing_unsafe_access_error` be set to false while we are within the outer caller. Although it’s fine since we will do a full stack walk in `is_accessing_session`, we should add a comment why this recursive case is okay (or save the old value as with `UnlockFlagSaver`). ------------- PR Review: https://git.openjdk.org/jdk/pull/27919#pullrequestreview-3411931601 PR Review Comment: https://git.openjdk.org/jdk/pull/27919#discussion_r2487147887 PR Review Comment: https://git.openjdk.org/jdk/pull/27919#discussion_r2487150214 PR Review Comment: https://git.openjdk.org/jdk/pull/27919#discussion_r2487152847
