FRF: 8033792: AltHashing used jint for imprecise bit shifting

[Calvin Cheung]

On 2/7/2014 10:23 AM, Yumin Qi wrote:
> 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'.
Instead of typecasting h1 back to jint before returning from 
AltHashing::murmur3_32(), I think it is better to change the function to 
return an unsigned int instead. It seems the callers of the function 
expects an unsigned int anyway.
C:\jdk9-hs-rt\src\share\vm\classfile\symbolTable.cpp(179): 
AltHashing::murmur3_32(seed(), (const jbyte*)s, len) :
   C:\jdk9-hs-rt\src\share\vm\classfile\symbolTable.cpp(605):  return 
use_alternate_hashcode() ? AltHashing::murmur3_32(seed(), s, len) :
   C:\jdk9-hs-rt\src\share\vm\memory\filemap.cpp(108):    unsigned int 
hash = AltHashing::murmur3_32(8191, (const jbyte*)vm_version, version_len);
   C:\jdk9-hs-rt\src\share\vm\oops\oop.cpp(112):    return 
AltHashing::murmur3_32(seed, chars, length);
   C:\jdk9-hs-rt\src\share\vm\oops\symbol.cpp(213):  return 
AltHashing::murmur3_32(seed, (const jbyte*)as_C_string(), utf8_length());

I haven't looked into the _seed being passed around as jint yet. It 
seems reasonable to change it to unsigned int too but it'll involve more 
code changes.

Calvin

>
>
> 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
>>
>