8353129: CDS ArchiveRelocation tests fail after JDK-8325132 #24308

calvinccheung · 2025-03-29T02:47:53Z

Two archive relocation tests failed when -XX:ArchiveRelocationMode=0 is specified via the jtreg -javaoption.
A fix is to add a WhiteBox.getArchiveRelocationMode() method so that the tests can check if the ArchiveRelocationMode is set to 0 before checking the expected output.

Passed tiers 1 - 4 testing.

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-8353129: CDS ArchiveRelocation tests fail after JDK-8325132 (Bug - P3)

Reviewers

Ioi Lam (@iklam - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24308

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-29T02:48:25Z

👋 Welcome back ccheung! 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-29T02:49:02Z

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

8353129: CDS ArchiveRelocation tests fail after JDK-8325132

Reviewed-by: iklam, dholmes

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

860a789: 8353219: RISC-V: Fix client builds after JDK-8345298
a83760a: 8352092: -XX:AOTMode=record crashes with InstanceKlass in allocated state
1077265: 8353321: [macos] ErrorTest.testAppContentWarning test case requires signing environment
52f56e6: 8353196: [macos] Contents of ".jpackage.xml" file are wrong when building .pkg from unsigned app image
acd4da4: 8353299: VerifyJarEntryName.java test fails
5eee32d: 8352768: CDS test MethodHandleTest.java failed in -Xcomp mode
8b0602d: 8319447: Improve performance of delayed task handling
fe8bd75: 8351290: Clarify integral only for vector operators
4d1de46: 8352185: Shenandoah: Invalid logic for remembered set verification
3e96f5c: 8351366: Remove the java.security.debug=scl option
... and 21 more: https://git.openjdk.org/jdk/compare/da3bb06797f9550d204b2623481ef757ae941bbd...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-29T02:49:33Z

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

hotspot

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-29T03:00:47Z

Webrevs

01: Full - Incremental (9bfa2ec6)
00: Full (94b6164b)

kimbarrett · 2025-03-30T01:05:48Z

src/hotspot/share/prims/whitebox.cpp

+  return (jint)ArchiveRelocationMode;
+#else
+  ShouldNotReachHere();
+  return (jint)-1;


Unnecessary dead code. Or maybe this shouldn't be using ShouldNotReachHere(). Is the intent
to crash in a debug build but return an error code in a product build? If so, ShouldNotReachHere()
doesn't provide that behavior, as it affects product builds too.

I think the intent is to crash if someone runs a test that uses CDS and this API in a build without CDS available. You literally should not reach here as the test should have been skipped at a higher level - that is true for debug and product test runs.

kimbarrett · 2025-03-30T01:09:28Z

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicArchiveRelocationTest.java

    public static void main(String... args) throws Exception {
+        WhiteBox wb = WhiteBox.getWhiteBox();


It seems this test already had WhiteBox enabled, but wasn't actually using it before this change.

Yes, the base class DynamicArchiveTestBase requires WhiteBox.

dholmes-ora · 2025-03-31T00:57:43Z

I think these tests are very confusing!

test/hotspot/jtreg/runtime/cds/appcds/ArchiveRelocationTest.java

States

@comment the test uses -XX:ArchiveRelocationMode=1 to force relocation.

but that is not what it does. It either sets -XX:ArchiveRelocationMode=0 in the exec'd VM or it relies on the default being 1 - which is not the case if it was set directly via JTREG. So it seems to me the right, and simple, fix here is to always pass the expected -XX:ArchiveRelocationMode value to the exec'd VM and ignore/override whatever comes in via the command-line.

calvinccheung · 2025-03-31T22:22:40Z

I think these tests are very confusing!

test/hotspot/jtreg/runtime/cds/appcds/ArchiveRelocationTest.java

States

@comment the test uses -XX:ArchiveRelocationMode=1 to force relocation.

but that is not what it does. It either sets -XX:ArchiveRelocationMode=0 in the exec'd VM or it relies on the default being 1 - which is not the case if it was set directly via JTREG. So it seems to me the right, and simple, fix here is to always pass the expected -XX:ArchiveRelocationMode value to the exec'd VM and ignore/override whatever comes in via the command-line.

I've simplified the fix based on your suggestions. The new change contains only test changes.

iklam

LGTM.

dholmes-ora

LGTM2. I hope it now passes tier4.

Thanks

dholmes-ora · 2025-03-31T00:42:54Z

src/hotspot/share/prims/whitebox.cpp

@@ -2131,6 +2131,15 @@ WB_ENTRY(jint, WB_GetCDSCurrentVersion(JNIEnv* env, jobject wb))
 #endif
 WB_END

+WB_ENTRY(jint, WB_GetArchiveRelocationMode(JNIEnv* env, jobject wb))
+#if INCLUDE_CDS
+  return (jint)ArchiveRelocationMode;


Nit: do we need casts between int and jint ??

The latest change doesn't involve whitebox.cpp.

calvinccheung · 2025-04-01T14:57:57Z

LGTM2. I hope it now passes tier4.

Yes, I re-ran tiers 1, 3, and 4 testing with the latest change.

calvinccheung · 2025-04-01T16:41:53Z

Thanks @iklam @dholmes-ora @kimbarrett for the review.

/integrate

openjdk · 2025-04-01T16:43:28Z

Going to push as commit 6a46d55.
Since your change was applied there have been 44 commits pushed to the master branch:

19eabaf: 8353227: JFR: Prepare tests for strong parser validation
564066d: 8353118: Deprecate the use of java.locale.useOldISOCodes system property
a1ab1d8: 8353449: [BACKOUT] One instance of STATIC_LIB_CFLAGS was missed in JDK-8345683
2a31f69: 8353331: Test ForkJoinPool20Test::testFixedDelaySequence is failing
1809138: 8352284: EXTRA_CFLAGS incorrectly applied to BUILD_LIBJVM src/hotspot C++ source files
cef5610: 8353272: One instance of STATIC_LIB_CFLAGS was missed in JDK-8345683
6801eb8: 8352709: Remove bad timing annotations from WhileOpTest.java
85a0baf: 8352719: Add an equals sign to the modules statement
f25f701: 8353226: JFR: emit old object samples must be transitive closure complete for segment
aff5aa7: 8350566: NMT: add size parameter to MemTracker::record_virtual_memory_tag
... and 34 more: https://git.openjdk.org/jdk/compare/da3bb06797f9550d204b2623481ef757ae941bbd...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-01T16:43:43Z

@calvinccheung Pushed as commit 6a46d55.

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

8353129: CDS ArchiveRelocation tests fail after JDK-8325132

3307177

openjdk bot added the hotspot [email protected] label Mar 29, 2025

whitespace error

94b6164

calvinccheung marked this pull request as ready for review March 29, 2025 02:56

openjdk bot added the rfr Pull request is ready for review label Mar 29, 2025

kimbarrett reviewed Mar 30, 2025

View reviewed changes

simplify the fix per David's suggestion

9bfa2ec

iklam approved these changes Mar 31, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 31, 2025

dholmes-ora approved these changes Apr 1, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 1, 2025

openjdk bot closed this Apr 1, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 1, 2025

calvinccheung deleted the 83553129-ArchiveRelocation-tests branch April 1, 2025 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8353129: CDS ArchiveRelocation tests fail after JDK-8325132 #24308

8353129: CDS ArchiveRelocation tests fail after JDK-8325132 #24308

calvinccheung commented Mar 29, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 29, 2025

openjdk bot commented Mar 29, 2025 •

edited

Loading

openjdk bot commented Mar 29, 2025

mlbridge bot commented Mar 29, 2025 •

edited

Loading

kimbarrett Mar 30, 2025

dholmes-ora Mar 31, 2025 •

edited

Loading

kimbarrett Mar 30, 2025

calvinccheung Mar 31, 2025

dholmes-ora commented Mar 31, 2025

calvinccheung commented Mar 31, 2025

iklam left a comment

dholmes-ora left a comment

dholmes-ora Mar 31, 2025

calvinccheung Apr 1, 2025

calvinccheung commented Apr 1, 2025

calvinccheung commented Apr 1, 2025

openjdk bot commented Apr 1, 2025

openjdk bot commented Apr 1, 2025

		public static void main(String... args) throws Exception {
		WhiteBox wb = WhiteBox.getWhiteBox();

8353129: CDS ArchiveRelocation tests fail after JDK-8325132 #24308

8353129: CDS ArchiveRelocation tests fail after JDK-8325132 #24308

Conversation

calvinccheung commented Mar 29, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 29, 2025

openjdk bot commented Mar 29, 2025 • edited Loading

openjdk bot commented Mar 29, 2025

mlbridge bot commented Mar 29, 2025 • edited Loading

Webrevs

kimbarrett Mar 30, 2025

Choose a reason for hiding this comment

dholmes-ora Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

kimbarrett Mar 30, 2025

Choose a reason for hiding this comment

calvinccheung Mar 31, 2025

Choose a reason for hiding this comment

dholmes-ora commented Mar 31, 2025

calvinccheung commented Mar 31, 2025

iklam left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Mar 31, 2025

Choose a reason for hiding this comment

calvinccheung Apr 1, 2025

Choose a reason for hiding this comment

calvinccheung commented Apr 1, 2025

calvinccheung commented Apr 1, 2025

openjdk bot commented Apr 1, 2025

openjdk bot commented Apr 1, 2025

calvinccheung commented Mar 29, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 29, 2025 •

edited

Loading

mlbridge bot commented Mar 29, 2025 •

edited

Loading

dholmes-ora Mar 31, 2025 •

edited

Loading