RFR: 8314225: SIGSEGV in JavaThread::is_lock_owned [v2]

[Dean Long]

On Tue, 30 Jan 2024 22:07:15 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Okay so anything looking at monitor_chunks is looking at a moving target. They have no idea what stage of moving from compiled to interpreted frames has been reached. So examining monitor_chunks just seems inherently unsafe and totally misguided. On the other hand if you want to know about all monitors then you need to know whether this deopt is in progress or not, and prevent it from starting or wait for it to finish.
>> 
>> But I also don't see how we examine monitors that are still in compiled frames? `is_lock_owned` does not consider them.
>> 
>> ??? This seems completely broken.
>
> @dholmes-ora said:
> 
>> But I also don't see how we examine monitors that are still in compiled frames? `is_lock_owned` does not consider them.
> 
> JavaThread::is_lock_owned() calls Thread::is_lock_owned() first to check if the lock record is on the native stack.

> @dean-long will be actually encounter this native addresses when looking at a monitor owner? Because the value of `adr` that we pass to `JavaThread::is_lock_owned()` is either the address that we read from the markword for the stack-locked case, or the value of the _owner field for the inflated monitor case. If I see `Deoptimization::relock_objects()`, the BasicLock* that we use to relock the monitor should be a valid stack address. Then when we "move" the monitors from the stack to the monitor chunk in [1], `BasicLock::move_to()` will inflate the lock but I don't see we are using the native destination address. Am I missing something here? In other words, do we even need to traverse this monitor chunks?

BasicLock::move_to() doesn't always inflate the lock.

First the lock record gets moved from the compiled frame to monitor chunk in `fill_in`, then from the monitor chunk to the interpreter frame in `unpack_on_stack`.  Assuming the monitor is not inflated during the move, and lacking additional synchronization, the code calling JavaThread::is_lock_owned() could have read the mark word while the monitor record was moved to the monitor chunks, right?

But it's still racy.  If the BasicLock gets moved to the interpreter after `JavaThread::is_lock_owned()` has already called `Thread::is_lock_owned()`, then it will return the wrong answer.  JavaThread::is_lock_owned is not prepared to deal with the lock moving.  That's why I said we might need a better solution.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17566#issuecomment-1918002042