On Tue, 11 Apr 2023 12:15:07 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
>> The NativeAccess is a left-over from an earlier attempt, and yes I think the >> start_processing() is the actual barrier. There is a single call-path where >> we inspect another thread's lock-stack outside of a safepoint (from >> management/JMX code). We had some arguments back and forth with David about >> that (somewhere up in this PR) and the conclusion so far is that yes, it is >> racy, but it doesn't seem to be a problem. We might be getting wrong results >> in the sense that the other thread could change the state of locking in the >> moment right after we inspect it, but this doesn't look like a correctness >> problem in the code that's calling it and the problem is pre-existing with >> current stack-locking, too. See jmm_GetThreadInfo() in management.cpp around >> lines 1129ff. > > It looks to me like the code could read racingly read the element just above > `_top`, which could contain a stale oop. If the address of the stale oop > matches the address of `o` then `contains` would incorrectly return true. > > Did you consider rewriting the racing code to use thread-local handshakes to > remove the race? Hmm you are right. But still - that problem is pre-existing, right? Consider this code in the current stack-locking implementation. If we don't stop the other thread, we may end up following a stack-pointer from the locked-object, and by the time we get to that stack-address, the thread may already have given up that lock and the stack-address could contain some other random stuff. Re-writing that code to do a handshake would be nice, but I don't think I want to include this in the scope of this PR. If you agree, I would file a separate issue to investigate the problem. As a band-aid, I could add a 'LockingMode == 2' to the if-statement in management.cpp as I already did it earlier: https://github.com/rkennke/jdk/blob/JDK-8291555-v2/src/hotspot/share/services/management.cpp#L1129. This would make all calls into LockStack::contains() happen at a safepoint or only by self-thread, and would certainly make me sleep a little better ;-) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1162848799
