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

[David Holmes]

On Tue, 30 Jan 2024 11:23:45 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:
> 
>   Add regression test

Many thanks for picking this up @xmas92 . 

I'm very frustrated that this code got broken the way it has been. Given there are only two options here: actual current thread or else a JVMTI suspended thread we are processing on-behalf of, I'm very tempted to introduce a new API for the latter case to use e.g.: `inflate_for` and `ObjectMonitor::set_owner_to`. When dealing with the suspended thread only very limited situations are actually possible so we don't need all the possible cases to be checked in `inflate` or `enter` - it can just be asserted what state the object/monitor must be in. We can then revert inflate/enter to always and only, act on the current thread. I think the resulting code would be much simpler to understand overall.

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

> 519: }
> 520: 
> 521: void ObjectSynchronizer::enter(Handle obj, BasicLock* lock, JavaThread* locking_thread, JavaThread* current) {

Do we actually need to pass in the current thread? What is it used for - ResourceMarks?

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

> 1310: // Can be called from non JavaThreads (e.g., VMThread) for FastHashCode
> 1311: // calculations as part of JVM/TI tagging.
> 1312: static bool is_lock_owned(JavaThread* locking_thread, oop obj) {

The parameter does not need renaming here, we are asking if some thread is the owner, it is not trying to lock anything. Also you've invalidated the comment by making this take a JavaThread instead of Thread.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17626#pullrequestreview-1852729256
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1472196825
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1472193929