FRF: 8033792: AltHashing used jint for imprecise bit shifting

[Yumin Qi]

David,
     We have a _seed which is defined as 'jint' but at use sites, it is 
used as 'unsigned int'. I am thinking change it to 'unsigned int', but 
don't know why in the start, it is defined as 'jint'. Anyone could point 
out the reason from begin it is defined as 'jint'?

   Look at this code:

   // Bucket handling
   int hash_to_index(unsigned int full_hash) {
     int h = full_hash % _table_size;
     assert(h >= 0 && h < _table_size, "Illegal hash value");
     return h;
   }

   The assert here with h>= 0 will be always true (? unless _table_size 
is negative).

   Other sites:

   // When String table needs to rehash
unsigned int oopDesc::new_hash(jint seed) {
   EXCEPTION_MARK;
   ResourceMark rm;
   int length;
   jchar* chars = java_lang_String::as_unicode_string(this, length, THREAD);
   if (chars != NULL) {
     // Use alternate hashing algorithm on the string
     return AltHashing::murmur3_32(seed, chars, length);
   } else {
     vm_exit_out_of_memory(length, OOM_MALLOC_ERROR, "unable to create 
Unicode strings for String table rehash");
     return 0;
   }
}

note murmur3_32 returns 'jint'.
All the usage contains conversion from 'jint' to 'unsigned int'.


Thanks
Yumin





On 2/7/2014 9:54 AM, Yumin Qi wrote:
>
> On 2/6/2014 11:47 PM, David Holmes wrote:
>> Hi Yumin,
>>
>> On 7/02/2014 2:14 PM, Yumin Qi wrote:
>>> Hi,
>>>
>>>    Please review the change for 8033792.
>>>
>>>    Summary:  AltHashing uses 'jint' type in the way as 'unsigned 
>>> int' in
>>> bit shifting, which is imprecise.  This could lead loss of precision
>>> when converted between jint and 'unsigned int' during bit operation. 
>>> Fix
>>> by changing operation variable type from 'jint' to 'juint', before
>>> return, cast it into type 'jint'.
>>>
>>> http://cr.openjdk.java.net/~minqi/8033792/webrev00/
>>
>> I don't see how casting to unsigned int can lead to a loss of 
>> precision here. Further you now assign h1 from a jint and you pass it 
>> as a jint parameter to the Integer_rotate functions which would seem 
>> to me to have more opportunity for conversion issues. Either way this 
>> code is completely confused about the type of arithmetic it is trying 
>> to do (and I would think a hash should be unsigned to begin with ??).
>>
> I will double check _seed, all the work here is related to it. If it 
> is defined as 'unsigned int' and all the operations on it with same 
> type, all the problems (complains from internal tool) should he gone.
>> You didn't mention the unrelated change in 
>> src/share/vm/oops/instanceKlass.hpp
> This is for the bug 8030129 which will not fix, but add an assert here 
> to catch problem in debug binary. Sorry forget to mention this.
>
> Thanks
> Yumin
>>
>> Thanks,
>> David
>>
>>> Thanks
>>> Yumin
>