RFR: 8324881: ObjectSynchronizer::inflate(Thread* current...) is invoked for non-current thread [v3]

[David Holmes]

On Wed, 31 Jan 2024 09:04:09 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> The `ObjectSynchronizer` has always assumed that the `current` parameters are both the current thread as well as the thread that is doing the locking. The only time that we are entering on behalf of another thread is when doing re-locking in deoptimization. This has worked because the deoptee thread is suspended. However ResourceMarks have been using the wrong thread when logging is enabled.
>> 
>> This change `ObjectSynchronizer` instruments the relevant methods with both a `JavaThread* locking_thread` as well as `[Java]Thread* current` to be able to use the correct thread for ResourceMarks. 
>> 
>> Having the `inflate` care about a `locking_thread` is a little unpleasant in my opinion. But it is required for LM_LIGHTWEIGHT. 
>> Would probably be cleaner if the inflate for LM_LIGHTWEIGHT was it's own thing, as it does not share the whole INFLATING protocol. But seems like a future RFE to refactor this code.
>> 
>> Can reproduce a crash by modifying `test/jdk/com/sun/jdi/EATests.java` and using `-XX:DiagnoseSyncOnValueBasedClasses=2` with LM_LEGACY or running `test/jdk/com/sun/jdi/EATests.java` with LM_LIGHTWEIGHT/LM_MONITOR and `-Xlog:monitorinflation=trace`. 
>> 
>> Could extend this test to capture this regression in the future (or creating a new test based on the same infrastructure). Will give it an attempt, so we have a regression test for this. But these tests get rather involved as the require a lot of jvmti setup.
>
> Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision:
> 
>   More restrictive API

src/hotspot/share/runtime/synchronizer.cpp line 449:

> 447: 
> 448: // Handle notifications when synchronizing on value based classes
> 449: void ObjectSynchronizer::handle_sync_on_value_based_class(Handle obj, JavaThread* locking_thread) {

Can we add a comment (or even repeat the assertion) that locking thread is either the current thread or a suspended thread, please. This method performs a  number of actions that are not safe to make on an arbitrary thread.

src/hotspot/share/runtime/synchronizer.cpp line 510:

> 508: // of this algorithm. Make sure to update that code if the following function is
> 509: // changed. The implementation is extremely sensitive to race condition. Be careful.
> 510: void ObjectSynchronizer::enter(Handle obj, BasicLock* lock, JavaThread* locking_thread) {

I suggested a new method here for the deopt thread, because I thought much of what is checked in this method is not applicable for the circumstances where we would be dealing with relocking during deopt - and thus the code would be much simpler if we had one method for the current thread, and a much simpler one for the deopt thread. Is that not the case?

src/hotspot/share/runtime/synchronizer.cpp line 1303:

> 1301:   if (LockingMode == LM_LIGHTWEIGHT && current->is_Java_thread()) {
> 1302:     return inflate_for(JavaThread::cast(current), obj, cause);
> 1303:   }

Not quite what I had in mind. I expected the deopt code to call `inflate_for` to make it clear it was a different kind of inflation request.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1473844172
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1473849021
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1473848932