RFR: 8221149: os::malloc checks MallocCatchPtr outside of ifdef ASSERT block

[Stefan Karlsson]

Thanks, David.

StefanK

On 2019-03-29 01:31, David Holmes wrote:
> Hi Stefan,
> 
> This looks good to me too.
> 
> Thanks,
> David
> 
> On 28/03/2019 10:33 pm, Stefan Karlsson wrote:
>> Hi Thomas,
>>
>> On 2019-03-28 09:21, Thomas Stüfe wrote:
>>> Hi Stefan,
>>>
>>> On Thu, Mar 28, 2019 at 8:47 AM Stefan Karlsson 
>>> <stefan.karlsson at oracle.com <mailto:stefan.karlsson at oracle.com>> wrote:
>>>
>>>     Hi all,
>>>
>>>     Please review this patch to move the MallocCatchPtr check into the
>>>     ifdef
>>>     ASSERT block, just like the other usages of it.
>>>
>>>     http://cr.openjdk.java.net/~stefank/8221149/webrev.01/
>>>     https://bugs.openjdk.java.net/browse/JDK-8221149
>>>
>>>
>>> Looks good. Note that you also could remove the test completely from 
>>> os::realloc for the oldptr!=NULL case (lines 764ff) since we 
>>> allocated using os::malloc(), which already does the test.
>>
>> Thanks for reviewing.
>>
>> I've incorporated your proposal:
>>   http://cr.openjdk.java.net/~stefank/8221149/webrev.02.delta
>>   http://cr.openjdk.java.net/~stefank/8221149/webrev.02
>>
>> StefanK
>>
>>>
>>>     A side note: Is the intention that MallocCatchPtr should find 
>>> pointers
>>>     to the memory address returned from ::malloc, or the memory 
>>> address we
>>>     hand out from os::malloc? Currently it's the latter and it's not
>>>     obvious
>>>     from the the code if this was the intention from the beginning.
>>>
>>>        704   // Wrap memory with guard
>>>        705   GuardedMemory guarded(ptr, size + nmt_header_size);
>>>        706   ptr = guarded.get_user_ptr();
>>>        707
>>>        708   if ((intptr_t)ptr == (intptr_t)MallocCatchPtr) {
>>>
>>>
>>> I find the test as it is is more useful since usually one wants to 
>>> follow pointers one sees in the hotspot.
>>>
>>> However, we may just test both pointers, yes? So, break if 
>>> MallocCatchPtr is either the outside or the inside pointer.
>>>
>>> I am not really emotionally invested though :)
>>>
>>> Cheers Thomas
>>>
>>>     Thanks,
>>>     StefanK
>>>