RFR (XXS): 8075288: malloc without free in VM_PopulateDumpSharedSpace::doit()

[Thomas Stüfe]

Hi,

Disclaimer: not a *r*eviewer.

The fix is simple and makes sense, even if it is not clear why saved_vtbl
is allocated. Would it in this case not make sense to at least fix the
memory leak?

Regards, Thomas


On Thu, Apr 30, 2015 at 4:35 PM, Jungwoo Ha <jwha at google.com> wrote:

> Can anyone approve this change?
>
> https://bugs.openjdk.java.net/browse/JDK-8075288
> http://cr.openjdk.java.net/~jwha/8075288/webrev.00
>
> Or at least tell me what to do.
>
> On Mon, Apr 20, 2015 at 5:23 PM, Jungwoo Ha <jwha at google.com> wrote:
>
> > Any update on this bug?
> > I think it is an obvious fix.
> >
> > On Fri, Apr 10, 2015 at 9:25 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
> >
> >> There's this comment right above the code:
> >>
> >>   // dunno what this is for.
> >>
> >> I also don't understand why we need to allocate a saved_vtbl, clear
> >> vtbl_list, and then vtbl_list later.  I think I tried removing these
> lines
> >> in the past and got into problems. So I'd suggest leaving them alone
> until
> >> you understand what really is going on.
> >>
> >> Thanks
> >> - Ioi
> >>
> >>
> >> On 4/10/15 8:00 AM, Mikael Gerdin wrote:
> >>
> >>> PopulateDumpSharedSpace is CDS, redirecting to hotspot-runtime-dev at ...
> >>>
> >>> On 2015-04-10 16:49, Carsten Varming wrote:
> >>>
> >>>> Dear Jungwoo,
> >>>>
> >>>> Why is this array not resource allocated?
> >>>>
> >>>
> >>> Actually, none of this really matters since the VM terminates after
> >>> executing the CDS dump. This has probably been done to satisfy some
> static
> >>> analysis tool that Google runs on the sources.
> >>>
> >>> /Mikael
> >>>
> >>>
> >>>> Carsten
> >>>>
> >>>> On Fri, Apr 10, 2015 at 9:31 AM, Jungwoo Ha <jwha at google.com
> >>>> <mailto:jwha at google.com>> wrote:
> >>>>
> >>>>     What's the conclusion? Should I fix it or discuss it separately?
> >>>>
> >>>>     On Thu, Apr 9, 2015 at 2:43 PM, Kim Barrett <
> kim.barrett at oracle.com
> >>>>     <mailto:kim.barrett at oracle.com>> wrote:
> >>>>
> >>>>         On Apr 9, 2015, at 1:25 PM, Jungwoo Ha <jwha at google.com
> >>>>         <mailto:jwha at google.com>> wrote:
> >>>>          >
> >>>>          > Can someone sponsor this change?
> >>>>          >
> >>>>          > https://bugs.openjdk.java.net/browse/JDK-8075288
> >>>>          > http://cr.openjdk.java.net/~jwha/8075288/webrev.00
> >>>>
> >>>>         I think the change is correct.  Good find!
> >>>>
> >>>>         However, it’s not clear to me why this is using malloc (and
> now
> >>>>         free), rather than stack allocation.
> >>>>         The size is a declared variable at the allocation point, but
> >>>>         it’s initialized from a constant with a
> >>>>         not big value (17) and never modified, so could be changed to
> be
> >>>>         a constant.
> >>>>
> >>>>         That is, instead of
> >>>>
> >>>>           592   char* saved_vtbl = (char*)os::malloc(vtbl_list_size *
> >>>>         sizeof(void*), mtClass);
> >>>>           593   memmove(saved_vtbl, vtbl_list, vtbl_list_size *
> >>>>         sizeof(void*));
> >>>>
> >>>>         use
> >>>>
> >>>>            void* saved_vtbl[vtbl_list_size];
> >>>>            memmove(saved_vtbl, vtbl_list, ARRAY_SIZE(saved_vtbl));
> >>>>
> >>>>         Maybe the worry is that vtbl_list_size might not always be a
> >>>>         small constant?
> >>>>
> >>>>         Probably this question of whether to use malloc/free here at
> all
> >>>>         should be a separate CR.
> >>>>
> >>>>
> >>>>
> >>>>
> >>
> >
>