RFR(S): 8069124 - runtime/NMT/MallocSiteHashOverflow.java failing in nightlies

[Coleen Phillimore]

In this case, I think 'long' is chosen because we want to turn address 
into an integral type to create the hashcode and that'll cover a 64 bit 
address.  Even if we change the signedness of the value, it's still a 
useful hash code.  I don't see a bug here, only unfortunate casts.

This change looks good to me.  It's a really good find, Christian! We've 
been chasing this bug for months.

Thanks,
Coleen


On 2/24/15, 11:09 PM, David Holmes wrote:
> Hi Christian,
>
> On 25/02/2015 12:57 PM, Christian Tornqvist wrote:
>> Hi everyone,
>>
>>
>>
>> Please review this small fix for an issue with NMT.
>
> The use of long in this function looks wrong, particularly as 
> sizeof(long) might equal sizeof(int) and long is signed but now 
> _hash_value is unsigned:
>
>  72 // Hash code. Any better algorithm?
>   73 unsigned int NativeCallStack::hash() const {
>   74   long hash_val = _hash_value;
>   75   if (hash_val == 0) {
>   76     long pc;
>   77     int  index;
>   78     for (index = 0; index < NMT_TrackingStackDepth; index ++) {
>   79       pc = (long)_stack[index];
>   80       if (pc == 0) break;
>   81       hash_val += pc;
>   82     }
>   83
>   84     NativeCallStack* p = const_cast<NativeCallStack*>(this);
>   85     p->_hash_value = (unsigned int)(hash_val & 0xFFFFFFFF);
>   86   }
>   87   return _hash_value;
>   88 }
>
> Even with the original code the use of long seems wrong.
>
> David
>
>
>>
>>
>> The failure was caused by reading random memory from the uninitialized
>> _hash_value, when this value happened to be 0x80000000, hash_to_index()
>> failed to negate the value and ended up with an index of -16:
>>
>>
>>
>> #  Internal Error
>> (C:\\jprt\\T\\P1\\130630.ctornqvi\\s\\hotspot\\src\\share\\vm\\services\\mal 
>>
>> locSiteTable.cpp:139), pid=5680, tid=4928
>>
>> #  assert(index >= 0) failed: Negative index -16
>>
>>
>>
>> Reproduced the issue and verified the fix using a debugger. Ran 
>> vm.quick and
>> hotspot/test/:hotspot_jprt tests on Linux i586/x64 and Windows 
>> i586/x64 with
>> -XX:NativeMemoryTracking=detail
>>
>>
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~ctornqvi/webrev/8069124/webrev.00/
>>
>>
>>
>> Bug:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8069124
>>
>>
>>
>> Thanks,
>>
>> Christian
>>
>>
>>