RFR: 8253495: CDS generates non-deterministic output [v2]

[David Holmes]

I can't find this comment in the PR so replying via email ...

On 11/03/2022 9:24 am, Ioi Lam wrote:
> On Wed, 9 Mar 2022 07:47:19 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
> 
>>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    Fixed zero build
>>
>> src/hotspot/share/utilities/hashtable.hpp line 42:
>>
>>> 40:
>>> 41:   LP64_ONLY(unsigned int _gap;)
>>> 42:
>>
>> For 64-bit, you now lose packing potential in the theoretical case the following payload does not have to be aligned to 64 bit. E.g. for T=char, where the whole entry would fit into 8 bytes. Probably does not matter as long as entries are allocated individually from C-heap which is a lot more wasteful anyway.
>>
>> For 32-bit, I think you may have the same problem if the payload starts with a uint64_t. Would that not be aligned to a 64-bit boundary too? Whether or not you build on 64-bit?
>>
>> I think setting the memory, or at least the first 8..16 bytes, of the entry to zero in BasicHashtable<F>::new_entry could be more robust:
>>
>> (16 bytes in case the payload starts with a long double but that may be overthinking it :)
>>
>>
>> template <MEMFLAGS F> BasicHashtableEntry<F>* BasicHashtable<F>::new_entry(unsigned int hashValue) {
>>    char* p = :new (NEW_C_HEAP_ARRAY(char, this->entry_size(), F);
>>    ::memset(p, 0, MIN2(this->entry_size(), 16)); // needs reproducable
>>    BasicHashtableEntry<F>* entry = ::new (p) BasicHashtableEntry<F>(hashValue);
>>    return entry;
>> }
>>
>> If you are worried about performance, this may also be controlled by a template parameter, and then you do it just for the system dictionary.
> 
> Thanks for pointing this out. I ran more tests and found that on certain platforms, there are other structures that have problems with uninitialized gaps. I ended up changing `os::malloc()` to zero the buffer when running with -Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't matter too much for regular VM executions because `os::malloc()` already has a high overhead.

This is raising red flags for me sorry. Every user of the JDK is now 
paying a penalty because of something only needed when dumping the 
shared archive. It might not be much but it is the old "death by a 
thousand cuts". Is there any way to tell the OS to pre-zero all memory 
provided to the current process, such that we could set that when 
dumping and not have to check on each allocation?

And I have to wonder how easy it would be to re-introduce 
non-deterministic values in these data structures that are being dumped. 
Does malloc itself even guarantee to return the same set of addresses 
for the same sequence of requests in different executions of a program?

Cheers,
David
-----

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/7748