On Thu, 5 Jun 2025 05:17:48 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> Radim Vansa has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Moved jtreg test
>> - Improved documentation
>> - Fix coding style (asterisk placement)
>
> I have written a POC that shows that the table must be sorted again when
> dumping a dynamic CDS archive. See
> https://github.com/iklam/jdk/commit/dcd53ebaeab7b38be02aa5b896ce9e449a45418f
>
> Explanations are in
> [here](https://github.com/iklam/jdk/commit/dcd53ebaeab7b38be02aa5b896ce9e449a45418f#diff-fd7608607ecf305bb3535b500bff5d53ec216d2da25e3bad1a1d699f56b09283R199)
>
> I will create an RFE for the JDK mainline that adds built-in debugging
> support for the `(oldSym > newSym_orig)` condition as describe in the POC.
> Please wait for that before integrating this PR. I can help you write the
> code for re-sorting the tables.
> As @iklam was saying in above, the table won't work for dynamic dumping for
> CDS and fails all these tests.
>
> ```
> Array<u1>* FieldInfoStream::create_search_table(ConstantPool* cp, const
> Array<u1>* fis, ClassLoaderData* loader_data, TRAPS) {
> + if (CDSConfig::is_dumping_dynamic_archive()) {
> + // We cannot call validate_search_table. The _fieldinfo_search_table
> should be sorted by "requested" addresses,
> + // but validate_search_table will be getting Symbol* addresses from
> _constants, which has "buffered" addresses.
> + //
> + // For background, see new comments inside allocate_node_impl in
> symbolTable.cpp
> + return nullptr;
> + }
> +
> ```
>
> This fixes these tests.
This fix looks good to me. It also obviates the need to re-sort the table for
dynamic CDS dump. I am OK with this. We can implement re-sorting for the
dynamic CDS archives in a separate RFE if desired.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/24847#issuecomment-2946819592
