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

[David Holmes]

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().

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
>>>