8351375: nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called #24040

lmesnik · 2025-03-13T19:02:08Z

The nsk_jvmti_setFailStatus() sometimes is called after test check results. In these cases the warning logs are generated and hide the real failure reasons. Also, I think it is a error-prone way to set and check error, since check might be just forgotten. Also, the test execution after failure might be incorrect and also make failure analysis harder.
So I think it makes sense to always fail when nsk_jvmti_setFailStatus() is called.

If this is going to work I'll rename it later and add add optional message to be more informative.

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-8351375: nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called (Bug - P4)

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer)
Chris Plummer (@plummercj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24040

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…th code 97 if fails in unload phase

bridgekeeper · 2025-03-13T19:02:34Z

👋 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 · 2025-03-13T19:02:55Z

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

8351375: nsk/jvmti/ tests should fail when  nsk_jvmti_setFailStatus() is called

Reviewed-by: sspitsyn, cjplummer

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

7442039: 8337279: Share StringBuilder to format instant
22182f7: 8352112: [ubsan] hotspot/share/code/relocInfo.cpp:130:37: runtime error: applying non-zero offset 18446744073709551614 to null pointer
c2e14b1: 8343891: Test javax/swing/JTabbedPane/TestJTabbedPaneBackgroundColor.java failed
52c6ce6: 8352091: GenShen: assert(!(request.generation->is_old() && _heap->old_generation()->is_doing_mixed_evacuations())) failed: Old heuristic should not request cycles while it waits for mixed evacuation
b8f3856: 8352420: [ubsan] codeBuffer.cpp:984:27: runtime error: applying non-zero offset 18446744073709486080 to null pointer
0cb110e: 8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields
04eac0c: 8352159: RISC-V: add more zfa support
ac760dd: 8352423: RISC-V: simplify DivI/L ModI/L
d1cf232: 8352248: Check if CMoveX is supported
2b55979: 8352529: RISC-V: enable loopopts tests
... and 170 more: https://git.openjdk.org/jdk/compare/7c9a7840ea75a9a7df3f240d063e9833f535e6ec...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 · 2025-03-13T19:03:39Z

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

serviceability

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 · 2025-03-13T19:06:24Z

Webrevs

01: Full - Incremental (5dae85f1)
00: Full (5982af70)

sspitsyn

The fix looks okay in general. I've posted one comment about typos and one question.

sspitsyn · 2025-03-19T21:10:09Z

test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/hotswap/HotSwap.cpp

+    if (phase == JVMTI_PHASE_DEAD) {
+      NSK_DISPLAY0("CompiledMethodLoad event recieved in dead phase");
+      return;
+    }


Nit: Typos at L148: GetMethodNamme => GetMethodName, is work => works, phasem => phase
A suggestion is to reformulate the comment as below:

// GetMethodName works in live phase only so just exit if the event is generated too late

Also, I wonder if we want to abort/fail in all cases when phase == JVMTI_PHASE_DEAD.

Also, I wonder if we want to abort/fail in all cases when phase == JVMTI_PHASE_DEAD.

I take it back. My suggestion does not look right.

comment fixed.
I haven't find in spec that event can't be generated in dead phase. So test should just ignore this event.

plummercj

Looks good.

sspitsyn

Thank you for the update! LGTM.

lmesnik · 2025-03-22T01:59:37Z

/integrate

openjdk · 2025-03-22T02:00:02Z

Going to push as commit 334a1ee.
Since your change was applied there have been 180 commits pushed to the master branch:

7442039: 8337279: Share StringBuilder to format instant
22182f7: 8352112: [ubsan] hotspot/share/code/relocInfo.cpp:130:37: runtime error: applying non-zero offset 18446744073709551614 to null pointer
c2e14b1: 8343891: Test javax/swing/JTabbedPane/TestJTabbedPaneBackgroundColor.java failed
52c6ce6: 8352091: GenShen: assert(!(request.generation->is_old() && _heap->old_generation()->is_doing_mixed_evacuations())) failed: Old heuristic should not request cycles while it waits for mixed evacuation
b8f3856: 8352420: [ubsan] codeBuffer.cpp:984:27: runtime error: applying non-zero offset 18446744073709486080 to null pointer
0cb110e: 8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields
04eac0c: 8352159: RISC-V: add more zfa support
ac760dd: 8352423: RISC-V: simplify DivI/L ModI/L
d1cf232: 8352248: Check if CMoveX is supported
2b55979: 8352529: RISC-V: enable loopopts tests
... and 170 more: https://git.openjdk.org/jdk/compare/7c9a7840ea75a9a7df3f240d063e9833f535e6ec...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-22T02:00:08Z

@lmesnik Pushed as commit 334a1ee.

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

dholmes-ora · 2025-03-23T22:06:39Z

There are a number of tier 5 failures after this change - filed JDK-8352652

Leonid Mesnik added 4 commits March 7, 2025 16:07

8351375: nsk/jvmti/scenarios/events/EM04/em04t001 test should exit wi…

9c0cb31

…th code 97 if fails in unload phase

set fail status should fails.

61f82dc

removed empty lines.

b1e6c4f

more fixed tests

Loading
Loading status checks…

5982af7

openjdk bot added the rfr label Mar 13, 2025

openjdk bot added the serviceability label Mar 13, 2025

lmesnik changed the title ~~8351375: nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called.~~ 8351375: nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called Mar 13, 2025

sspitsyn reviewed Mar 19, 2025

View reviewed changes

comment updated

Loading
Loading status checks…

5dae85f

plummercj approved these changes Mar 20, 2025

View reviewed changes

openjdk bot added the ready label Mar 20, 2025

sspitsyn approved these changes Mar 22, 2025

View reviewed changes

openjdk bot added the integrated label Mar 22, 2025

openjdk bot closed this Mar 22, 2025

openjdk bot removed ready rfr labels Mar 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8351375: nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called #24040

8351375: nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called #24040

lmesnik commented Mar 13, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 13, 2025

openjdk bot commented Mar 13, 2025 •

edited

Loading

openjdk bot commented Mar 13, 2025

mlbridge bot commented Mar 13, 2025 •

edited

Loading

sspitsyn left a comment

sspitsyn Mar 19, 2025

sspitsyn Mar 20, 2025 •

edited

Loading

lmesnik Mar 22, 2025

plummercj left a comment

sspitsyn left a comment

lmesnik commented Mar 22, 2025

openjdk bot commented Mar 22, 2025

openjdk bot commented Mar 22, 2025

dholmes-ora commented Mar 23, 2025

8351375: nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called #24040

8351375: nsk/jvmti/ tests should fail when nsk_jvmti_setFailStatus() is called #24040

Conversation

lmesnik commented Mar 13, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 13, 2025

openjdk bot commented Mar 13, 2025 • edited Loading

openjdk bot commented Mar 13, 2025

mlbridge bot commented Mar 13, 2025 • edited Loading

Webrevs

sspitsyn left a comment

Choose a reason for hiding this comment

sspitsyn Mar 19, 2025

Choose a reason for hiding this comment

sspitsyn Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

lmesnik Mar 22, 2025

Choose a reason for hiding this comment

plummercj left a comment

Choose a reason for hiding this comment

sspitsyn left a comment

Choose a reason for hiding this comment

lmesnik commented Mar 22, 2025

openjdk bot commented Mar 22, 2025

openjdk bot commented Mar 22, 2025

dholmes-ora commented Mar 23, 2025

lmesnik commented Mar 13, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 13, 2025 •

edited

Loading

mlbridge bot commented Mar 13, 2025 •

edited

Loading

sspitsyn Mar 20, 2025 •

edited

Loading