RFR: 8317262: LockStack::contains(oop) fails "assert(t->is_Java_thread()) failed: incorrect cast to JavaThread"

[Stefan Karlsson]

On Thu, 5 Oct 2023 08:41:44 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Please review this simple fix to `LockStack::is_owning_thread()` that allows it to be called by a non-JavaThread. Thanks to @pchilano  for the regression test.
>> 
>> Testing:
>> - new regression test
>> - runtime/handshake/MixedHandshakeWalkStackTest.java
>> - tiers 1-3 sanity
>> 
>> Thanks
>
> FWIW, there's a similar static function in sychronizer.cpp:
> 
> // Can be called from non JavaThreads (e.g., VMThread) for FastHashCode
> // calculations as part of JVM/TI tagging.
> static bool is_lock_owned(Thread* thread, oop obj) {
>   assert(LockingMode == LM_LIGHTWEIGHT, "only call this with new lightweight locking enabled");
>   return thread->is_Java_thread() ? JavaThread::cast(thread)->lock_stack().contains(obj) : false;
> }
> 
> 
> There might be an opportunity to combine/reuse/deduplicate this implementation.

> Thanks for looking at this @stefank !
> 
> > FWIW, there's a similar static function in sychronizer.cpp
> 
> They are not really that similar. Both are interested only in JavaThread's but one checks the current thread and the other checks a target thread.

Still. There's something fishy going on when the call sites look like this:
is_lock_owned => LockStack::contains => is_owning_thread

and both is_lock_owned and is_owning_thread filters away non-JavaThreads.

IMHO, I think that the entire StackWatermark handling should be pulled out of LockStack. Code that inspects oops of other threads need to start StackWatermark processing of the target thread. The StackWatermark processing inside LockStack looks like a workaround because some handshake operation forgot to do so. The patch seems to solve the immediate failure is OK to push, but I'd like to have a follow-up discussion on why we have this StackWatermark processing here.

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

PR Comment: https://git.openjdk.org/jdk/pull/16047#issuecomment-1750252281