8374639: Static archive with AOTClassLinking breaks dynamic archive

[iklam]

Bug fix

The assert happens because JDK-8309240 (Array classes should be stored in dynamic CDS archive) is not compatible with AOTClassLinking since JDK 26.

Since the benefit of JDK-8309240 is small (obj array classes can be quickly created at run time), we should remove JDK-8309240.

Background

The incompatibility is caused by JDK-8350550 (Preload classes from AOT cache during VM bootstrap) added in JDK 26.

JDK-8309240 adds support for scenarios like this:

• class Foo is in the static archive, but Foo[] is not in the static archive
• class Foo[] is in the dynamic archive

There's a pointer from the InstanceKlass of Foo to its array class Foo[] (InstanceKlass::_array_klasses). With JDK-8309240, we update this pointer to point to Foo[] right after the CDS archives are mapped. This happens before vmClasses::resolve_all():

jdk/src/hotspot/share/cds/aotMetaspace.cpp

Line 2162 in dd20e91

DynamicArchive::setup_array_klasses();

With JDK-8350550, if the static archive has AOT-linked classes, we will restore all of its classes inside vmClasses::resolve_all() -- see the callstack in the Description of JDK-8374639.

The class restoration requires that all the classes (both instance classes and their array classes) must have a Java mirror. In this example, Foo is from the static archive and has an archived Java mirror, so we are OK. However, Foo[] is from the dynamic archive and doesn't have an archived mirror. Also, we are too early in the bootstrap process so we are unable to allocate a Java mirror from the heap yet. Hence the assert.

---

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-8374639: Static archive with AOTClassLinking breaks dynamic archive (Bug - P4)

Reviewers

• Coleen Phillimore (@coleenp - Reviewer)
• Matias Saavedra Silva (@matias9927 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 29078

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back iklam! 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]

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

8374639: Static archive with AOTClassLinking breaks dynamic archive

Reviewed-by: coleenp, matsaave

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

• 9513758: 8374662: Remove unused type check functions from javaClasses.hpp
• 70669d0: 8374712: AOTMappedHeapWriter::relocate_field_in_buffer should use CompressedOops::narrow_oop_cast
• 0a1fa21: 8369150: NMethodRelocationTest fails when JVMTI events not published before JVM exit
• ... and 15 more: https://git.openjdk.org/jdk/compare/6b3c1e0f786a889d2ac25c8bd05f4d83e666425f...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]

@iklam 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 (1ce56f99)

[coleenp]

So for the static archive, there will still be InstanceKlass::_array_klasses saved, just not for dynamic archive?
This looks really good. It is easy to recreate array classes on demand as they're needed.

[matias9927]

This change looks good! It will also help in lworld considering we ran into similar problems with the array metadata refactor. Thanks!

[iklam]

So for the static archive, there will still be InstanceKlass::_array_klasses saved, just not for dynamic archive? This looks really good. It is easy to recreate array classes on demand as they're needed.

After this PR, we can still store some ArrayKlasses in both the static archive and the dynamic archive, and all the pointers InstanceKlass::_array_klasses, ArrayKlass::_higher_dimension, ArrayKlass::_lower_dimension, ObjArrayKlass::_element_klass, and ObjArrayKlass::_bottom_klass and set properly inside the archives.

However, we will no longer have an ArrayKlass in the dynamic archive whose element type is in the static archive. Therefore, none of the above pointers will be pointing from one archive to the other.

(All the code removed from this PR had to do with setting up such cross-archive pointers).

E.g., if you run this app with -XX:ArchiveClassesAtExit

class Hello {
    public static void main(String[] args) {
        System[] x = new System[0]; 
        System.out.println(x);
        Hello[] y = new Hello[0];
        System.out.println(y);
    }
}

• System[] will not be stored in the dynamic archive (because its element type System is in the static archive).
• Hello[] will be stored in the dynamic archive (because its element type Hello is in the dynamic archive).

[iklam]

Thanks @coleenp and @matias9927 for the review
/integrate

[openjdk]

Going to push as commit 9fd86e3.
Since your change was applied there have been 34 commits pushed to the master branch:

• 1fb5030: 8374767: Amend JDK-8374521 with new option name
• 92abc6d: 8369282: Distrust TLS server certificates anchored by Chunghwa ePKI Root CA
• afd216e: 8374752: Add more JLS links to javax.lang.model.element.*
• ... and 31 more: https://git.openjdk.org/jdk/compare/6b3c1e0f786a889d2ac25c8bd05f4d83e666425f...master

Your commit was automatically rebased without conflicts.

[openjdk]

@iklam Pushed as commit 9fd86e3.

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