RFR: 8366154: Validate thread type requirements in debug commands [v2]
Kerem Kat
krk at openjdk.org
Tue Sep 9 09:30:27 UTC 2025
On Mon, 8 Sep 2025 09:40:58 GMT, Kevin Walls <kevinw at openjdk.org> wrote:
>> It is not ideal but the original
>>
>> if (!c.onJavaThread()) return;
>> JavaThread* p = JavaThread::active();
>>
>> is also duplicating the internals both inside `isOnJavaThread()` and in the callers. And what is mainly tripping us up is that `active()` is allowed to return null, but this fix wants to detect a null current thread as a sign of being detached. So we end up calling `Thread::current` multiple times. All of the debug functions that use `active()` are currently broken anyway because they never check it for null (and I'm not even sure they truly expect to work on a delegated JavaThread??). So I see this fix as having two parts:
>> 1. The `onThread` check takes care of being attached
>> 2. The `active() == nullptr` check just fixes the current broken code.
>>
>> But I still dislike the internal duplication between `onThread` and `active`.
>
> Have used these occasionally and am surprised to realize psf and others show frames from the VM operation's caller thread.
>
>
> The duplication around the active() checks is odd. If we are adding:
>
> bool onThread()
> If no current thread, print warning and return false. Callers will return immediately.
>
> Could we add e.g.:
>
> bool hasActiveThread()
> If no current thread, print warning and return false. Callers will return immediately.
> If no active thread, print warning and return false. Callers will return immediately.
> Callers can go on to use JavaThread* p = JavaThread::active(); without checking (as we currently do).
> (We are presumably in a debugger session, speed is not the priority, repeating some checks or fetching of active() is not tragic, but this file has less duplication.)
Thank you for the description for `hasActiveThread`. Is it much different than the `onJavaThread` in the first revision?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27033#discussion_r2332718975
More information about the hotspot-dev
mailing list