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

[Thomas Stüfe]

Hi Stefan,

all good, thank you.

Cheers, Thomas

On Thu, Mar 28, 2019 at 1:33 PM Stefan Karlsson <stefan.karlsson at oracle.com>
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
> >
>