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

[David Holmes]

Looks good.

Thanks,
David

On 10/03/2015 1:29 AM, Christian Tornqvist wrote:
> 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
>>>>>
>>>>>
>>>>>
>>>
>
>