RFR: 8330027: Identity hashes of archived objects must be based on a reproducible random seed [v5]

[Ioi Lam]

On Wed, 1 May 2024 11:37:48 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> CDS archive contains archived objects with identity hashes.
>> 
>> These hashes are deliberately preserved or even generated during dumping. They are generated based on a seed that is initialized randomly on a per-thread basis. These generations precede CDS dump initialization, so they are not affected by the init_random call there, nor would they be affected by [JDK-8323900](https://bugs.openjdk.org/browse/JDK-8323900).
>> 
>> A random seed will not work for dumping archives since it prevents reproducible archive generation. Therefore, when dumping, these seeds must be initiated in a reproducible way.
>> 
>> --- Update
>> 
>> After discussions with Ioi, and several redos, we settled on:
>> 
>> - make sure CDS dump only ever calls ihash generation from one thread. Means, we disable the explicit hash generation we had been doing before, since that one was called from the VM thread, not from the single java thread
>> - Start out with a constant seed for all threads. This is fine and does not cause collisions between threads, since - see above - we only call ihash generation from a single threads.
>> - We also assert that we only use a single thread
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit:
> 
>   JDK-8330027-cds-ihash-reproducability.patch

Changes requested by iklam (Reviewer).

src/hotspot/share/runtime/synchronizer.cpp line 952:

> 950:   if (t == nullptr) {
> 951:     Atomic::cmpxchg(&cds_dump_java_thread, t, cur);
> 952:     return true;

I think this is not thread safe, as I got few intermittent asserts.

src/hotspot/share/runtime/synchronizer.cpp line 1011:

> 1009:     // ever calls ihash
> 1010:     assert(!CDSConfig::is_dumping_archive() || runs_on_one_thread_only(),
> 1011:         "Only one thread should generate ihash during CDS dumps");

It should check for `CDSConfig::is_dumping_static_archive()` instead (here and in `Thread::Thread()`), to match the check in [JVM_StartThread]()https://github.com/openjdk/jdk/blob/7c1fad4fb6c387bbfb72b3f96b610e7cbc2ef312/src/hotspot/share/prims/jvm.cpp#L2946-L2948)

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

PR Review: https://git.openjdk.org/jdk/pull/18735#pullrequestreview-2037422157
PR Review Comment: https://git.openjdk.org/jdk/pull/18735#discussion_r1588758940
PR Review Comment: https://git.openjdk.org/jdk/pull/18735#discussion_r1588757725