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

[Mikael Gerdin]

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