RFR(XXS) 8249087 Symbol constructor unnecessarily initializes _body[0]

[Ioi Lam]

On 7/20/20 11:12 PM, Florian Weimer wrote:
> * Ioi Lam:
>
>> Hi please review this very simple fix:
>>
>> diff -r 19f26d72a8d0 src/hotspot/share/oops/symbol.cpp
>> --- a/src/hotspot/share/oops/symbol.cpp    Mon Jul 20 14:24:19 2020 -0700
>> +++ b/src/hotspot/share/oops/symbol.cpp    Mon Jul 20 17:11:57 2020 -0700
>> @@ -51,8 +51,11 @@
>>   Symbol::Symbol(const u1* name, int length, int refcount) {
>>     _hash_and_refcount = pack_hash_and_refcount((short)os::random(),
>> refcount);
>>     _length = length;
>> -  _body[0] = 0;  // in case length == 0
>>     memcpy(_body, name, length);
>> +  // For symbols of length 0 and 1: _body[0] (and _body[1]) are
>> uninitialized and may
>> +  // contain random values, which will only be read by
>> Symbol::identity_hash(),
>> +  // which would tolerate such randomness. These values never change
>> during the lifetime
>> +  // of the Symbol.
>>   }
> Won't this still trip memory debuggers?  Symbol::identity_hash() implies
> that the result is eventually used in a conditional operation (a hash
> comparison perhaps).  If it's possible one day to run Hotspot under
> valgrind, this would result in false positives.

Are you saying that valgrind will modify uninitialized memory 
periodically after the constructor has returned, and thus will cause 
Symbol::identity_hash() to return a different value?

Without my patch, _body[1] is uninitialized for Symbols whose length is 
0 or 1. We have not heard of any issues related to valgrind and 
Symbol::identity_hash().

In fact, looking at the code history, the setting of "_body[0] = 0" in 
Symbol::Symbol was introduced only recently (Feb 2020):

http://hg.openjdk.java.net/jdk/jdk/annotate/4a4d185098e2/src/hotspot/share/oops/symbol.cpp#l55

I'll check with Lois who added the code to see the reason for doing it.


Thanks
- Ioi

> Thanks,
> Florian
>