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

[Christian Tornqvist]

Changed the long to a uintptr_t and cleaned up the loop a bit.

Please see the update webrev at:
http://cr.openjdk.java.net/~ctornqvi/webrev/8069124/webrev.02/

Thanks,
Christian

-----Original Message-----
From: hotspot-runtime-dev
[mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Coleen
Phillimore
Sent: Wednesday, February 25, 2015 11:50 PM
To: David Holmes; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(S): 8069124 - runtime/NMT/MallocSiteHashOverflow.java
failing in nightlies


On 2/25/15, 11:04 PM, David Holmes wrote:
> On 26/02/2015 1:55 PM, Coleen Phillimore wrote:
>>
>>
>> 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.
>
> Sounds like a job for intptr_t.

Or maybe uintptr_t if they're arithmetic.
>
>> 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.
>
> I'm surprised this change doesn't induce further warnings about mixing 
> signed and unsigned. But the code screams "type confusion" to me.
>
>> This change looks good to me.  It's a really good find, Christian! 
>> We've been chasing this bug for months.
>
> The real bug was reading an uninitialized value no? In which case the 
> change from signed to unsigned seems incidental, unless also guarding 
> against overflow.

There were two bugs.  The signed hash value was 0x8000000 and when negated
was 0x8000000, so created a negative index into the hash table.

Coleen
>
> David
> -----
>
>>
>> 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\\serv
>>>> ices\\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
>>>>
>>>>
>>>>
>>