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

[Ioi Lam]

On Tue, 18 Feb 2025 22:22:30 GMT, Calvin Cheung <ccheung 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/filemap.cpp line 1389:
> 
>> 1387:   const char* file_type = CDSConfig::type_of_archive_being_loaded();
>> 1388:   if (_is_static) {
>> 1389:     if ((gen_header->_magic == CDS_ARCHIVE_MAGIC) ||
> 
> Probably don't need the extra set of parentheses.

Since the next line has a more complex condition, I think putting both this line and the next line in parenthesis makes the code easier to read.

> src/hotspot/share/cds/finalImageRecipes.hpp line 67:
> 
>> 65: 
>> 66: public:
>> 67:   static void serialize(SerializeClosure* soc, bool is_static_archive);
> 
> The only caller is from `MetaspaceShared::serialize(`):
>     `FinalImageRecipes::serialize(soc, true);`
> Wondering if the `is_static_archive` arg is needed?

I removed the `is_static_archive` flag.

> src/hotspot/share/cds/metaspaceShared.cpp line 819:
> 
>> 817:   CDSConfig::DumperThreadMark dumper_thread_mark(THREAD);
>> 818:   ResourceMark rm(THREAD);
>> 819:   HandleMark hm(THREAD);
> 
> Why do we need HandleMark?

This is not necessary. I removed it.

> src/hotspot/share/cds/metaspaceShared.cpp line 839:
> 
>> 837:       tty->print_cr("AOTConfiguration recorded: %s", AOTConfiguration);
>> 838:       vm_exit(0);
>> 839:     } else {
> 
> Is it appropriate to add assert of `CDSConfig::is_dumping_final_static_archive()` in the `else` case?

In the `else` case, we could be dumping either "final" or "classic" static archive. This function is invoked only when dumping static archives, so I think extra asserts aren't necessary here.

> src/hotspot/share/cds/metaspaceShared.cpp line 958:
> 
>> 956: 
>> 957:   if (CDSConfig::is_dumping_preimage_static_archive()) {
>> 958:     log_info(cds)("Reading lambda form invokers of in JDK default classlist ...");
> 
> Suggestion:
>     "Reading lambda form invokers from JDK default classlist ...."

Fixed.

> src/hotspot/share/classfile/systemDictionaryShared.cpp line 995:
> 
>> 993: 
>> 994:   int length = record->num_verifier_constraints();
>> 995:   if (length > 0 || klass->name()->equals("HelloWorld")) {
> 
> Is the "HelloWorld" check leftover from debugging?

Fixed.

> src/hotspot/share/classfile/systemDictionaryShared.cpp line 1031:
> 
>> 1029: 
>> 1030:   int length = rt_info->num_verifier_constraints();
>> 1031:   if (length > 0 || klass->name()->equals("HelloWorld")) {
> 
> Is the "HelloWorld" check leftover from debugging?

Fixed.

> src/hotspot/share/classfile/systemDictionaryShared.cpp line 1164:
> 
>> 1162:   JavaThread* current = JavaThread::current();
>> 1163:   if (klass->is_shared_platform_class() || klass->is_shared_app_class()) {
>> 1164:     DumpTimeClassInfo* dt_info = get_info(klass);
> 
> `dt_info` seems unused.

Removed.

> test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/AOTLoaderConstraintsTest.java line 80:
> 
>> 78:         public void checkExecution(OutputAnalyzer out, RunMode runMode) throws Exception {
>> 79:             switch (runMode) {
>> 80:             case RunMode.ASSEMBLY:   // JEP 485 + binary AOTConfiguration -- should load AppClass from preimage
> 
> s/485/483

Fixed

> test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/AOTLoaderConstraintsTest.java line 101:
> 
>> 99:         // AppClass is loaded by the app loader. To make sure that you cannot use
>> 100:         // type masquerade attacks, we need to add a loader constraint that says:
>> 101:         //  app and loo loaders must resolve the symbol "java/lang/String" to the same type.
> 
> Suggestion:
> 
> // app and _boot_ loaders ...

Fixed.

> test/lib/jdk/test/lib/cds/CDSAppTester.java line 365:
> 
>> 363:     }
>> 364: 
>> 365:     // See JEP 485
> 
> s/485/483

Fixed

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962560521
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962560973
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962560625
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962560688
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962560741
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962560792
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962560857
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962560924
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962561090
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962561140
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1962561025