On Tue, 11 Apr 2023 13:48:15 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> 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 ;-) 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1162872406
