8379913: [lworld] AppCDS test asserts with -Xcomp --enable-preview

[dafedafe]

Issue

Many AppCDS test asserts with assert(false) failed: Address 0x00007f0dfc2923e0 for <unknown>/('verify_oop: r11: broken oop oop_result, "broken oop in call_VM_base" (src/hotspot/cpu/x86/macroAssembler_x86.cpp:1353)') is missing in AOT Code Cache addresses table when run with -Xcomp --enable-preview

Cause

The crash happens during AOT cache dumping seemingly because -XX:+VerifyOops causes the adapter to use addresses that the AOT doesn't know about. In particular, verify_oop/verify_oop_addr add a message C‑string and reference the verify‑oop stub entry. AOTCodeCache::write_relocations() tries to serialize those relocations, AOTCodeAddressTable::id_for_address() can’t resolve them and crashes.
In this case the issue happens with --enable-preview because it creates an adapter for scalarized arguments and, after creating the oop from the arguments, get_vm_result_oop invokes verify_oop_msg. Without --enable-preview the path is never taken and the missing "registration" isn’t exercised (I fear that this crash could potentially be triggered by some other (non preview) code but the fix doesn't need to distinguish between preview/non-preview).

Fix

Instead of making the two addresses used by verify_oop_msg "visible" to AOT we disable AOT code generation if both VerifyOops and InlineTypePassFieldsAsArgs are true.

Testing

Tier 1-3+
Failing CDS tests before and after

---

Progress

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

Issue

• JDK-8379913: [lworld] AppCDS test asserts with -Xcomp --enable-preview (Bug - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - no project role)
• Tobias Hartmann (@TobiHartmann - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2233

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2233.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

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

8379913: [lworld] AppCDS test asserts with -Xcomp --enable-preview

Reviewed-by: kvn, thartmann

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 2 new commits pushed to the lworld branch:

• 357956b: 8377670: [lworld] RO adapter signature should not scalarize inline receiver in CompiledEntrySignature::initialize_from_fingerprint()
• 2bb7e72: 8380197: [lworld] [REDO] OptimizePtrCompare is too conservative

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld 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 lworld branch, type /integrate in a new comment.

[dafedafe]

@vnkozlov I think it would be good if could have a quick look at this.

[mlbridge]

Webrevs

• 03: Full - Incremental (3793fba6)
• 02: Full - Incremental (40625f55)
• 01: Full - Incremental (8588e531)
• 00: Full (cacde664)

[vnkozlov]

The fix is correct. Good work.

We actually disable AOT code generation when VerifyOops is on. It produces a lot of C strings and we have only fixed number of slots (500 in debug VM) to record them.

If you can manually run locally test, please run with -Xlog:aot+codecache+exit=debug it will show (among other things) how many C strings were recorded in AOT cache during -XXAOTMode=record phase:

[3.139s][debug][aot,codecache,exit]   Wrote 1 C strings of total length 22 

I don't want to hit 500 limit.

[vnkozlov]

An other solution would be to disable AOT code generation if VerifyOops is on:

diff --git a/src/hotspot/share/code/aotCodeCache.cpp b/src/hotspot/share/code/aotCodeCache.cpp
index 68db21850aa..cb7098f8641 100644
--- a/src/hotspot/share/code/aotCodeCache.cpp
+++ b/src/hotspot/share/code/aotCodeCache.cpp
@@ -188,14 +188,14 @@ void AOTCodeCache::initialize() {
   FLAG_SET_ERGO(AOTStubCaching, false);
 
   if (VerifyOops) {
-    // Disable AOT stubs caching when VerifyOops flag is on.
+    // Disable AOT code caching when VerifyOops flag is on.
     // Verify oops code generated a lot of C strings which overflow
     // AOT C string table (which has fixed size).
     // AOT C string table will be reworked later to handle such cases.
     //
-    // Note: AOT adapters are not affected - they don't have oop operations.
-    log_info(aot, codecache, init)("AOT Stubs Caching is not supported with VerifyOops.");
-    FLAG_SET_ERGO(AOTStubCaching, false);
+    log_info(aot, codecache, init)("AOT Code Caching is not supported with VerifyOops.");
+    disable_caching();
+    return;
   }
 
   bool is_dumping = false;

[dafedafe]

Thank you @vnkozlov!

Disabling the AOT seems actually a more sensible solution (even though my tests showed just a few C strings recorded) but I've only added the disabling of the adapter part (FLAG_SET_ERGO(AOTAdapterCaching, false);) instead of disabling all the caching. Does it make sense?

[TobiHartmann]

That looks like a reasonable fix to me but it's only needed when preview is enabled, right?

[dafedafe]

That looks like a reasonable fix to me but it's only needed when preview is enabled, right?

Yep, more precisely only when InlineTypePassFieldsAsArgs is true. Added that condition.

[TobiHartmann]

Looks good to me!

[vnkozlov]

Good.

[dafedafe]

Thank you @vnkozlov @TobiHartmann for your reviews.

[dafedafe]

/integrate

[openjdk]

Going to push as commit f12276a.
Since your change was applied there have been 4 commits pushed to the lworld branch:

• e0c8622: 8369808: [lworld] 8366837 partially reverted duing 26+15 merge
• ee1e278: 8379800: [lworld] Do not run TestOopsInReturnConvention.java and TestVirtualThreads.java with Xcomp
• 357956b: 8377670: [lworld] RO adapter signature should not scalarize inline receiver in CompiledEntrySignature::initialize_from_fingerprint()
• ... and 1 more: https://git.openjdk.org/valhalla/compare/ee48773888ced8e22149ea519028611001563030...lworld

Your commit was automatically rebased without conflicts.

[openjdk]

@dafedafe Pushed as commit f12276a.

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