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

[Ioi Lam]

On Sun, 23 Feb 2025 17:24:21 GMT, Ashutosh Mehra <asmehra at openjdk.org> wrote:

>> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits:
>> 
>>  - Merge branch 'master' into 8348426-binary-aot-config-file
>>  - @ashu-mehra comments
>>  - @calvinccheung comments
>>  - 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
>>  - Fixed test cases @vnkozlov
>>  - Update "make test JTREG_AOT_JDK=true ..." to work with binary AOT configuration
>>  - Fixed test failures
>>  - Added comments; fixed FIXMEs
>>  - Added more test cases
>>  - ... and 2 more: https://git.openjdk.org/jdk/compare/00d4e4a9...21f140e7
>
> src/hotspot/share/cds/cppVtables.cpp line 200:
> 
>> 198: //     _orig_cpp_vtptrs[ConstantPool_Kind] == ((intptr_t**)cp)[0]
>> 199: //
>> 200: // _archived_cpp_vtptrs is a map of all the vptprs used by classes in a preimage. E.g., for
> 
> Thanks for adding these comments. I think I now understand why we need `_archived_cpp_vtptrs`.
> I am wondering if we really need to store this table in the preimage.
> When the control enters `CppVtables::dumptime_init`, if we are dumping the final archive, then the `_index[kind].cloned_vtable()` would be pointing to the vtable in the preimage. So we can initialize the `_archived_cpp_vtptrs` at that time before `_index[kind]` is overwritten by the runtime vtable data.
> Wouldn't that work?
> 
> Something like this:
> 
> ```@@ -231,13 +231,15 @@ char* CppVtables::_vtables_serialized_base = nullptr;
>  void CppVtables::dumptime_init(ArchiveBuilder* builder) {
>    assert(CDSConfig::is_dumping_static_archive(), "cpp tables are only dumped into static archive");
>  
> -  CPP_VTABLE_TYPES_DO(ALLOCATE_AND_INITIALIZE_VTABLE);
> -
> -  if (!CDSConfig::is_dumping_final_static_archive()) {
> +  // When dumping final archive, _index[kind] at this point is in the preimage.
> +  // Store the vtable pointers present in the preimage  as _index[kind] will now be rewritten
> +  // to point to the runtime vtable data.
> +  if (CDSConfig::is_dumping_final_static_archive()) {
>      for (int kind = 0; kind < _num_cloned_vtable_kinds; kind++) {
>        _archived_cpp_vtptrs[kind] = _index[kind]->cloned_vtable();
>      }
>    }
> +  CPP_VTABLE_TYPES_DO(ALLOCATE_AND_INITIALIZE_VTABLE);
>  
>    size_t cpp_tables_size = builder->rw_region()->top() - builder->rw_region()->base();
>    builder->alloc_stats()->record_cpp_vtables((int)cpp_tables_size);
> @@ -253,16 +255,6 @@ void CppVtables::serialize(SerializeClosure* soc) {
>    if (soc->reading()) {
>      CPP_VTABLE_TYPES_DO(INITIALIZE_VTABLE);
>    }
> -
> -  if (soc->writing() && !CDSConfig::is_dumping_preimage_static_archive()) {
> -    // This table is written only when creating the preimage. It will be used
> -    // only when writing the final static archive.
> -    memset(_archived_cpp_vtptrs, 0, sizeof(_archived_cpp_vtptrs));
> -  }
> -
> -  for (int kind = 0; kind < _num_cloned_vtable_kinds; kind++) {
> -    soc->do_ptr(&_archived_cpp_vtptrs[kind]);
> -  }
>  }

Thanks for the suggestion. I've incorporated your patch and added an assert. I also fixed `CppVtables::is_valid_shared_method()` to use `_archived_cpp_vtptrs`

While fixing the code, I found some typos in near-by comments that are also unclear, so I rewrote the comment about `_vtables_serialized_base `

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1968658522