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

[Axel Boldt-Christmas]

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

Created a more restrictive external API with no overloads.

API changes
```C++
  // Now explicitly takes the locking_thread, does not conflate with current.
  static void enter(Handle obj, BasicLock* lock, JavaThread* locking_thread);
  // Unchanged behaviour
  static ObjectMonitor* inflate(Thread* current, oop obj, const InflateCause cause);
  // Used with LM_LIGHTWEIGHT to inflate a monitor with another threads lock_stack
  static ObjectMonitor* inflate_for(JavaThread* thread, oop obj, const InflateCause cause);
  // Now explicitly takes the locking_thread, does not conflate with current.
  static void handle_sync_on_value_based_class(Handle obj, JavaThread* locking_thread);

  // Internal. Shared inflate implementation, LM_LIGHTWEIGHT will inflate with the specific lock_stack
  //                if provided.
  static ObjectMonitor* inflate_impl(LockStack* lock_stack, oop obj, const InflateCause cause);


As for the internal API using the LockStack* makes it more clear that this is a LM_LIGHTWEIGHT parameter.

There is still an API issue here, and an assert-ability issue w.r.t. `ObjectSynchronizer::enter` and `ObjectMonitor::enter`. When entering on behalf of another thread the call to `ObjectMonitor::enter` must succeed without contention, that is it is either locked the the other thread and appears recursive, or the object has not escaped and cannot have contention. 

A solution could be to have a `ObjectMonitor::enter_prologue` which returns false if there is contention. Which can be caught and asserted. And have `ObjectMonitor::enter` only accept `JavaThread::current()`. 

These are all specialised APIs which only exists for re-lock. 

The current implementation is correct, but it is still not nice to have a `JavaThread* current` which does not `== JavaThread::current()`.

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

PR Comment: https://git.openjdk.org/jdk/pull/17626#issuecomment-1918698516