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

[David Holmes]

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