RFR: 8348426: Generate binary file for -XX:AOTMode=record -XX:AOTConfiguration=file [v4]

[Ioi Lam]

On Wed, 19 Feb 2025 03:42:12 GMT, Ashutosh Mehra <asmehra at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Improved JTREG_AOT_JDK=true so we do not need to add test code into the JDK itself
>>  - Improve error message when AOTMode=create has an incompatible classpath
>
> src/hotspot/share/cds/archiveBuilder.cpp line 510:
> 
>> 508:   } else if (klass->is_objArray_klass()) {
>> 509:     Klass* bottom = ObjArrayKlass::cast(klass)->bottom_klass();
>> 510:     if (CDSConfig::is_dumping_dynamic_archive() && MetaspaceShared::is_shared_static(bottom)) {
> 
> Why do we have the check for `CDSConfig::is_dumping_dynamic_archive()` here?

Before this PR, `CDSConfig::is_dumping_dynamic_archive()` was the only reason we will see `MetaspaceShared::is_shared_static(bottom)`, and clearly it wasn't excluded (it was included in the static archive).

After this PR, `bottom` can come from the mapped shared archive (the preimage). It's logically the same as a regular class we've loaded in a classic static dump, so it should go through the `SystemDictionaryShared::is_excluded_class()` check.

> src/hotspot/share/cds/archiveUtils.inline.hpp line 70:
> 
>> 68: // Returns the address of an Array<T> that's allocated in the ArchiveBuilder "buffer" space.
>> 69: template <typename T>
>> 70: Array<T>* ArchiveUtils::archive_ptr_array(GrowableArray<T>* tmp_array) {
> 
> If I am reading this code correctly it requires that the elements in `tmp_array` have already been archived. Can we add a comment and/or an assert to that effect.

I added comments about what the elements can be.


// All pointers in tmp_array must point to:
//    - a buffered object; or
//    - a source object that has been archived; or
//    - (only when dumping dynamic archive) an object in the static archive.


If it's not one of these types, it will be caught by the assert inside `builder->get_buffered_addr(ptr)`

> src/hotspot/share/cds/cdsConfig.cpp line 550:
> 
>> 548: 
>> 549: bool CDSConfig::is_dumping_preimage_static_archive() {
>> 550:   return _is_dumping_static_archive && _is_dumping_preimage_static_archive;
> 
> Is the check for `_is_dumping_static_archive` really needed?

I removed it. `_is_dumping_static_archive` is always set to true when `_is_dumping_preimage_static_archive` is set to true.

> src/hotspot/share/cds/cdsConfig.cpp line 705:
> 
>> 703: bool CDSConfig::is_dumping_aot_linked_classes() {
>> 704:   if (is_dumping_preimage_static_archive()) {
>> 705:     return false;
> 
> In leyden-premain branch it returns `AOTClassLinking`, but here it is returning false. So we are not doing pre-linking in the preimage, is that right?

Right, in this PR prelinking is not done in preimage. When we load the preimage to dump the final image, `FinalImageRecipes::load_all_classes()` is responsible for loading all classes.

The Leyden repo enables prelinking in the preimage. It relies on AOTLinkedClassBulkLoader to load all the classes when creating the final image. However, this is unsafe because we don't have the archived module graph there. I plan to fix the Leyden code when this PR is merged into Leyden.

> src/hotspot/share/cds/filemap.cpp line 1529:
> 
>> 1527:   // allow processes that have it open continued access to the file.
>> 1528:   remove(_full_path);
>> 1529:   int mode = CDSConfig::is_dumping_preimage_static_archive() ? 0666 : 0444;
> 
> Why do we need to give different access permission for preimage file compared to other dumping modes?

It's to keep the current behavior: -XX:AOTConfiguration creates a file with read/write permission but -XX:AOTCache creates a read-only file.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962846991
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962846956
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962847058
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962847101
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962847143