RFR: 8268902: Testing for threadObj != NULL is unnecessary in suspend handshake

[Coleen Phillimore]

On 6/28/21 1:31 AM, David Holmes wrote:
> Never mind ...
>
> On 28/06/2021 3:21 pm, David Holmes wrote:
>> On 26/06/2021 8:43 am, David Holmes wrote:
>>> On 26/06/2021 3:57 am, Coleen Phillimore wrote:
>>>> In the last pull request on this topic, I was making more general 
>>>> (mis)statements, but this null threadObj check is only for the case 
>>>> where we're suspending a thread which already has a threadObj.  
>>>> This null check is unnecessary.
>>>
>>> This is probably true, but possibly only because we screen out 
>>> jni-attaching threads at a higher-level.
>>
>> I can't quite join all the dots here. If we look at JDWP allThreads 
>> command, it will call JVM TI GetAllThreads, which uses:
>>
>>    // enumerate threads (including agent threads)
>>    ThreadsListEnumerator tle(current_thread, true);
>>
>> which is implicitly
>>
>>    ThreadsListEnumerator tle(current_thread, true, true);
>>
>> from:
>>
>>    ThreadsListEnumerator(Thread* cur_thread,
>>                          bool include_jvmti_agent_threads = false,
>>                          bool include_jni_attaching_threads = true);
>>
>> so in this case we will return those threads that are still attaching 
>> and may have no threadObj().
>
> I overlooked that before we decide to include (or not) JNI attaching 
> threads, we have already filtered out any thread with a NULL threadObj().

I was looking further down the call stack to the jvmti_SuspendThread

static jvmtiError JNICALL
jvmti_SuspendThread(jvmtiEnv* env,
             jthread thread) {

But interestingly, this ThreadsListEnumerator also filters out threadObj 
== NULL because it returns the list of threadObj.

Thanks for digging deeper.  You usually find what I miss!  Thanks
Coleen


>
> So no issues.
>
> Sorry for the noise.
>
> David
> -----
>
>> Can such a thread then be used by the JDWP Suspend() command ... it 
>> means we will get a NULL in the list of jthreads, and the first thing 
>> we will try to do is call  JVM TI GetThreadLocalStorage - which is 
>> specified to treat NULL as "use the current thread"! So that seems 
>> like a bug - but not the bug this PR is concerned about. :)
>>
>> I'll follow this up with serviceability folk.
>>
>> Cheers,
>> David
>>
>>
>>> Cheers,
>>> David
>>>
>>>> Tested already with tier1-6 (rerun tier1).
>>>>
>>>> -------------
>>>>
>>>> Commit messages:
>>>>   - 8268902: Testing for threadObj != NULL is unnecessary in 
>>>> suspend handshake
>>>>
>>>> Changes: https://git.openjdk.java.net/jdk/pull/4598/files
>>>>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4598&range=00
>>>>    Issue: https://bugs.openjdk.java.net/browse/JDK-8268902
>>>>    Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
>>>>    Patch: https://git.openjdk.java.net/jdk/pull/4598.diff
>>>>    Fetch: git fetch https://git.openjdk.java.net/jdk 
>>>> pull/4598/head:pull/4598
>>>>
>>>> PR: https://git.openjdk.java.net/jdk/pull/4598
>>>>