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

[coleenp]

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.
Tested already with tier1-6 (rerun tier1).

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8268902: Testing for threadObj != NULL is unnecessary in suspend handshake

Reviewers

• Patricio Chilano Mateo (@pchilano - Committer)
• Daniel D. Daugherty (@dcubed-ojdk - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4598/head:pull/4598
$ git checkout pull/4598

Update a local copy of the PR:
$ git checkout pull/4598
$ git pull https://git.openjdk.java.net/jdk pull/4598/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4598

View PR using the GUI difftool:
$ git pr show -t 4598

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4598.diff

[bridgekeeper]

👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@coleenp The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (5c3cdb61)

[pchilano]

LGTM.

[dcubed-ojdk]

Thumbs up.

[openjdk]

@coleenp This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: pchilanomate, dcubed

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 6 new commits pushed to the master branch:

• 68ef21d: 4847239: (spec) File.createTempFile() should make it clear that it doesn't create the temporary directory
• 3fae4b3: 6633375: FileOutputStream_md.c should be merged into FileOutputStream.c
• 223759f: 8266901: Clarify the method description of Duration.toDaysPart()
• 35c4702: 8268967: Update java.security to use switch expressions
• b565459: 8267138: Stray suffix when starting gtests via GTestWrapper.java
• 1d16797: 8268469: Update java.time to use switch expressions

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[coleenp]

Thanks Patricio and Dan. Patricio thank you for the offline discussion and sanity check :)

[mlbridge]

Mailing list message from David Holmes on hotspot-runtime-dev:

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.

Cheers,
David

[mlbridge]

Mailing list message from David Holmes on hotspot-runtime-dev:

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.

Cheers,
David

[mlbridge]

Mailing list message from David Holmes on hotspot-runtime-dev:

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

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

[mlbridge]

Mailing list message from David Holmes on hotspot-runtime-dev:

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

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

[mlbridge]

Mailing list message from David Holmes on hotspot-runtime-dev:

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

[mlbridge]

Mailing list message from David Holmes on hotspot-runtime-dev:

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

[mlbridge]

Mailing list message from Coleen Phillimore on hotspot-runtime-dev:

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

[mlbridge]

Mailing list message from Coleen Phillimore on hotspot-runtime-dev:

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

[coleenp]

Thanks for the code reviews and investigations. Sorry the gitbot changed the punctuation on my email to question marks.
/integrate

[openjdk]

Going to push as commit 29bc381.
Since your change was applied there have been 43 commits pushed to the master branch:

• 87ff277: 8269222: Incorrect number of workers reported for reference processing
• 4d2412e: 8269122: The use of "extern const" for Register definitions generates poor code
• f45be15: 8269003: Update the java manpage for JDK 18
• a29953d: Merge
• d9cb068: 8258746: illegal access to global field _jvmci_old_thread_counters by terminated thread causes crash
• 6eb734a: 8266269: Lookup::accessClass fails with IAE when accessing an arrayClass with a protected inner class as component class
• 3d0d27c: 8269351: Proxy::newProxyInstance and MethodHandleProxies::asInterfaceInstance should reject sealed interfaces
• 824a516: 8269260: Add AVX512 and other SSE + AVX combinations testing for tests which generate vector instructions
• 1404e4b: 8269302: serviceability/dcmd/framework/InvalidCommandTest.java still fails after JDK-8268433
• fb0a95f: 8269036: tools/jpackage/share/AppImagePackageTest.java failed with "hdiutil: create failed - Resource busy"
• ... and 33 more: https://git.openjdk.java.net/jdk/compare/ffa34ed429079827c9b0acb843288fde4b7fa53d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@coleenp Pushed as commit 29bc381.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.