On Fri, 31 Mar 2023 15:24:07 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> Roman Kennke has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'origin/JDK-8291555-v2' into JDK-8291555-v2 >> - Check underflow, top-of-stack and mark-bits for sanity, in fast_unlock() >> (aarch64) > > src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 6264: > >> 6262: ldrw(t1, Address(rthread, JavaThread::lock_stack_top_offset())); >> 6263: cmpw(t1, (unsigned)LockStack::start_offset()); >> 6264: br(Assembler::GT, stack_ok); > > I had to think hard about "GT" here. > > We could have entered with the thread holding just one inflated lock, then > LockStack would be empty but the monitorexit would still be valid. You now do > check in the callers for markWord::monitor_value. But the lock could have > been inflated concurrently after the caller checks and before this point. > > But then the LockStack would not have changed, since it represents what the > current thread *thinks* are thin locks, not what are actually thin locks? In > other words, LockStack is only modified by its owning thread, never from the > outside. > > So this *should* be correct, but its certainly a brain teaser. Maybe add a > comment? > > E.g. "These checks rely on the fact that LockStack is only ever modified by > its owning stack, even if the lock got inflated concurrently; removal of > LockStack entries after inflation will happen delayed in that case" or > somesuch. This also mandates that fast_lock can only ever entered if the current thread thinks that the lock in question is a thin lock. So the caller checks for markWord::monitor_value are mandatory now. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1154619603
