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

[David Holmes]

On Fri, 2 Feb 2024 15:11:19 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:
> 
>   Improve EARelockingNestedInflated_03 for LM_LIGHTWEIGHT

A few minor comment tweaks. Thanks

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

> 516: 
> 517:     // An async deflation can race after the inflate_for() call and before
> 518:     // enter() can make the ObjectMonitor busy. enter_for() returns false if

s/enter()/enter_for()/ ?

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

> 587:           return true;
> 588:         }
> 589:         // Fall through to inflate() ...

This comment and the one at line 579 are no longer clear because we don't inflate in this function.

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

> 1408:       bool own = inflating_thread != nullptr && inflating_thread->lock_stack().contains(object);
> 1409:       if (own) {
> 1410:         // Owned by us.

"us" is not appropriate when no longer dealing with the current thread.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17626#pullrequestreview-1861739571
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1477572871
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1477574300
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1477575900