On Tue, 11 Apr 2023 14:04:17 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
>> 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 ;-) > > OK. Given that I haven't looked at the rest of the patch, I leave it up to > you and the Reviewers to figure out what to do about this code. Cheers. Given that the race with new lightweight locking is virtually the same as the race with legacy stack locking, please do not put back the 'LockingMode == 2' check which would make `jmm_GetThreadInfo()` calls slower with new lightweight locking than with legacy stack locking. Perhaps I'm not understanding the risk of what @stefank means with: 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1162994635
