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

[Dean Long]

On Thu, 2 May 2024 19:40:18 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

>> Removal of JavaThread's MonitorChunks member.  This held lock information during deoptimization, but access to it is unnecessary for anything other than the deoptimization itself.
>> 
>> Access to it in is_lock_owned() was racy, and caused rare crashes.
>
> Kevin Walls has updated the pull request incrementally with one additional commit since the last revision:
> 
>   monitor->owner() == nullptr handling in fill_in

src/hotspot/share/runtime/vframeArray.cpp line 94:

> 92:         assert(!monitor->owner_is_scalar_replaced() || realloc_failures, "object should be reallocated already");
> 93:         BasicObjectLock* dest = _monitors->at(index);
> 94:         if (monitor->owner_is_scalar_replaced() || monitor->owner() == nullptr) {

The only way to get a null owner is if owner_is_scalar_replaced() is true:
https://github.com/openjdk/jdk/blob/6bef0474c8b8773d0d20c0f25c36a2ce9cdbd7e8/src/hotspot/share/runtime/stackValue.hpp#L52
and to get this far with it still null means `realloc_failures` is true.  We could avoid a null check later in unpack_on_stack if we skip adding to _monitors in this case.  So maybe use a GrowableArray inside MonitorChunk and add elements using append().
Suggestion:

        if (monitor->owner_is_scalar_replaced()) {

src/hotspot/share/runtime/vframeArray.cpp line 316:

> 314:     BasicObjectLock* src = _monitors->at(index);
> 315:     top->set_obj(src->obj());
> 316:     assert(src->obj() == nullptr || ObjectSynchronizer::current_thread_holds_lock(thread, Handle(thread, src->obj())),

No need for null check if we don't add null owners to _monitors.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18940#discussion_r1588615873
PR Review Comment: https://git.openjdk.org/jdk/pull/18940#discussion_r1588616143