RFR: 8366154: Validate thread type requirements in debug commands [v2]

[Kevin Walls]

On Mon, 8 Sep 2025 06:31:58 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> @dholmes-ora, are you sure you want this level of code duplication? As is it, the latest version duplicates the following 3 times
>> 
>>  if (!c.onThread()) return;
>>   JavaThread* p = JavaThread::active();
>>   if (p == nullptr) {
>>     tty->print_cr("Failed: JavaThread::active is null");
>>     return;
>>   }
>>   tty->print(" for thread: ");
>>   p->print();
>>   tty->cr();
>
> 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.)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27033#discussion_r2329709411