8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state #19275

lmesnik · 2024-05-17T01:48:30Z

The JvmtiTrace::safe_get_thread_name sometimes crashes when called while current thread is in native thread state.

It happens when thread_name is set for tracing from jvmti functions.
See:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEnter.xsl#L649

The setup is called and the thread name is used in tracing before the thread transition. There is no good location where this method could be called from vm thread_state only. Some functions like raw monitor enter/exit never transition in vm state. So sometimes it is needed to call this function from native thread state.

The change should affect JVMTI trace mode only (-XX:TraceJVMTI).

Verified by running jvmti/jdi/jdb tests with tracing enabled.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 12ddfca2
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19275/head:pull/19275
$ git checkout pull/19275

Update a local copy of the PR:
$ git checkout pull/19275
$ git pull https://git.openjdk.org/jdk.git pull/19275/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19275

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19275.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-17T01:49:22Z

👋 Welcome back lmesnik! 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 · 2024-05-17T01:50:14Z

@lmesnik 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:

8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state

Reviewed-by: dholmes, sspitsyn

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 142 new commits pushed to the master branch:

673f767: 8285506: Unify os::vsnprintf implementations
91ab088: 8333116: test/jdk/tools/jpackage/share/ServiceTest.java test fails
9ac8d05: 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache'
91caec0: 8330542: Template for Creating Strict JAXP Configuration File
da6aa2a: 8332849: Update doc/testing.{md,html} (spelling and stale information)
b8f2ec9: 8195675: Call to insertText with single character from custom Input Method ignored
0f3e2cc: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal
51ae08f: 8333093: Incorrect comment in zAddress_aarch64.cpp
4754f05: 8333035: Parallel: Remove ParMarkBitMap::IterationStatus
87a06b6: 8325805: Compiler Implementation for Flexible Constructor Bodies (Second Preview)
... and 132 more: https://git.openjdk.org/jdk/compare/7c750fd95b83d0a93b0cce681dcfbbae1f220fdd...master

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.

openjdk · 2024-05-17T01:50:46Z

@lmesnik The following labels will be automatically applied to this pull request:

hotspot
serviceability

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

mlbridge · 2024-05-17T01:53:14Z

Webrevs

dholmes-ora

I have to wonder whether this solution will potentially cause problems because the code will now block for safepoints. We could fallback to Thread::name() if the current thread is in-native.

lmesnik · 2024-05-17T16:53:01Z

I have to wonder whether this solution will potentially cause problems because the code will now block for safepoints. We could fallback to Thread::name() if the current thread is in-native.

Thanks for feedback. Here is the update.

I've updated the safe_get_thread_name() to not change thread state.

In "jvmtiEnter.xsl" functions the thread name is s read once before the transition happened and re-used then.
So I updated the tracing to 're-read' if the transition to VM happened to update the thread name once it became known.

dholmes-ora · 2024-05-19T22:22:30Z

I don't understand the additional changes because they read the current thread's name, whereas this issue is about reading an arbitrary thread's name when the current thread happens to be in the wrong state. ???

dholmes-ora

Okay now I get it. Even once the function is made truly safe, we are always calling it from an unsafe state and so will get the default Thread::name response. So now, after any transition to the VM the name is read again to get a good value. This seems a good enhancement though I have to wonder if the apparent changing of the thread name in the tracing might cause problems. The tracing really needs to include a unique thread identifier.

Thanks

dholmes-ora · 2024-05-20T03:44:44Z

src/hotspot/share/prims/jvmtiEventController.cpp

@@ -958,7 +958,6 @@ JvmtiEventControllerPrivate::clear_to_frame_pop(JvmtiEnvThreadState *ets, JvmtiF
 void
 JvmtiEventControllerPrivate::change_field_watch(jvmtiEvent event_type, bool added) {
  int *count_addr;
-


Nit: this file doesn't need to be touched.

AlanBateman · 2024-05-20T05:59:02Z

Are there tests that run with TraceJVMTI so that this option is tested?

lmesnik · 2024-05-20T17:10:13Z

There are no tests currently executed with TraceJVMTI.
I am thinking about adding execution of the svc testing. However I've got another failure that should resolved before
https://bugs.openjdk.org/browse/JDK-8332536
It is not related to the current issue.
Probably, it makes sense to add some basic logging testing with verification of log content also.

lmesnik · 2024-05-28T22:25:58Z

I discussed the issue with @sspitsyn and after this, I updated the function to return something more descriptive in case we can't read thread state. (Easier to understand what happens.)

sspitsyn

This looks good, Posted one nit though.

sspitsyn · 2024-05-29T01:18:57Z

src/hotspot/share/prims/jvmtiTrace.cpp

+  if (Thread::current()->is_Java_thread()) {
+    JavaThreadState current_state = JavaThread::cast(Thread::current())->thread_state();
+    if (current_state == _thread_in_native || current_state == _thread_blocked) {
+      return "not readable";


Nit: I'd suggest to make it more detailed, something like like this:
"" or ""

Yes this would have looked better if the text was more clearly an error message with angle brackets.

@sspitsyn, @dholmes-ora Thanks for the naming suggestion, looks to long in the report. Let me try to use logging and see if it makes sense to make more improvements.

lmesnik · 2024-05-29T14:58:20Z

The name becomes too long in the logs.

lmesnik · 2024-05-29T14:58:28Z

/integrate

openjdk · 2024-05-29T14:59:20Z

Going to push as commit 03b7a85.
Since your change was applied there have been 153 commits pushed to the master branch:

43a2f17: 8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage
fed2b56: 8320999: RISC-V: C2 RotateLeftV
6cda4c5: 8321543: Update NSS to version 3.96
c003c12: 8331865: Consolidate size and alignment checks in LayoutPath
6d718ae: 8324341: Remove redundant preprocessor #if's checks
9b64ece: 8332904: ubsan ppc64le: c1_LIRGenerator_ppc.cpp:581:21: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long int'
3d4eb15: 8302744: Refactor Hotspot container detection code
2cca83b: 8332880: JFR GCHelper class recognizes "Archive" regions as valid
b8ae11e: 8332960: ubsan: classListParser.hpp:159:12: runtime error: load of value 2101478704, which is not a valid value for type 'ParseMode'
9a83dfe: 8332431: NullPointerException in JTable of SwingSet2
... and 143 more: https://git.openjdk.org/jdk/compare/7c750fd95b83d0a93b0cce681dcfbbae1f220fdd...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-29T14:59:26Z

@lmesnik Pushed as commit 03b7a85.

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

lmesnik added 2 commits May 16, 2024 18:27

8332259

3caf988

include updated.

a2b1942

openjdk bot added the rfr Pull request is ready for review label May 17, 2024

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels May 17, 2024

lmesnik changed the title ~~8332259: JvmtiTrace::safe_get_thread_name is not safe~~ 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state May 17, 2024

lmesnik added 2 commits May 16, 2024 19:04

Merge branch 'master' of https://github.com/openjdk/jdk into 8332259

d3c4049

copyrights updated.

c534c91

dholmes-ora reviewed May 17, 2024

View reviewed changes

don't change state

f8fd474

wrong thread state

12ddfca

dholmes-ora approved these changes May 20, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 20, 2024

lmesnik added 2 commits May 28, 2024 15:21

The result is updated.

5630024

fixed space.

81dc407

sspitsyn approved these changes May 29, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 29, 2024

openjdk bot closed this May 29, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state #19275

8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state #19275

lmesnik commented May 17, 2024 •

edited by openjdk bot

bridgekeeper bot commented May 17, 2024

openjdk bot commented May 17, 2024 •

edited

openjdk bot commented May 17, 2024

mlbridge bot commented May 17, 2024 •

edited

dholmes-ora left a comment

lmesnik commented May 17, 2024

dholmes-ora commented May 19, 2024

dholmes-ora left a comment

dholmes-ora May 20, 2024

AlanBateman commented May 20, 2024

lmesnik commented May 20, 2024

lmesnik commented May 28, 2024

sspitsyn left a comment

sspitsyn May 29, 2024

dholmes-ora May 30, 2024

lmesnik Jun 3, 2024

lmesnik commented May 29, 2024

lmesnik commented May 29, 2024

openjdk bot commented May 29, 2024

openjdk bot commented May 29, 2024

8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state #19275

8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state #19275

Conversation

lmesnik commented May 17, 2024 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 17, 2024

openjdk bot commented May 17, 2024 • edited

openjdk bot commented May 17, 2024

mlbridge bot commented May 17, 2024 • edited

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

lmesnik commented May 17, 2024

dholmes-ora commented May 19, 2024

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora May 20, 2024

Choose a reason for hiding this comment

AlanBateman commented May 20, 2024

lmesnik commented May 20, 2024

lmesnik commented May 28, 2024

sspitsyn left a comment

Choose a reason for hiding this comment

sspitsyn May 29, 2024

Choose a reason for hiding this comment

dholmes-ora May 30, 2024

Choose a reason for hiding this comment

lmesnik Jun 3, 2024

Choose a reason for hiding this comment

lmesnik commented May 29, 2024

lmesnik commented May 29, 2024

openjdk bot commented May 29, 2024

openjdk bot commented May 29, 2024

lmesnik commented May 17, 2024 •

edited by openjdk bot

openjdk bot commented May 17, 2024 •

edited

mlbridge bot commented May 17, 2024 •

edited